feat: campaigns API (Cairo) - #18
Conversation
📝 WalkthroughWalkthroughA comprehensive campaign creation feature is introduced with Cairo/StarkNet blockchain integration. This includes new database entities (campaigns and audit logs), a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as Campaign Controller
participant Service as Campaign Service
participant CairoClient as Cairo Client
participant Starknet as Starknet Chain
participant DB as Database
Client->>Controller: POST /api/v1/campaigns<br/>(JWT auth header)
Controller->>Controller: Verify JWT & extract userId
Controller->>Controller: Validate request schema
Controller->>Service: createCampaign(userId, walletAddress,<br/>campaignRef, targetAmount, donationToken)
Service->>DB: Check duplicate campaignRef
alt Duplicate Found
Service-->>Controller: Error (DUPLICATE_CAMPAIGN_REF)
else Proceed
Service->>DB: Load wallet by address
alt Wallet Missing
Service-->>Controller: Error (WALLET_NOT_FOUND)
else Wallet Found
Service->>Service: Validate wallet balance
alt Insufficient Balance
Service-->>Controller: Error (INSUFFICIENT_BALANCE)
else Balance OK
Service->>CairoClient: createCampaign(campaignRef,<br/>targetAmount, donationToken)
alt Mock Mode
CairoClient-->>CairoClient: Generate mock hash & ID
else Starknet Mode
CairoClient->>Starknet: Execute create_campaign<br/>on factory contract
Starknet-->>CairoClient: Transaction hash
CairoClient->>Starknet: Wait for transaction<br/>receipt & events
Starknet-->>CairoClient: Event logs with campaignId
CairoClient-->>CairoClient: Parse CAMPAIGN_CREATED_EVENT
end
CairoClient-->>Service: {transactionHash, campaignId}
Service->>DB: Save CampaignEntity
Service->>DB: Save AuditLogEntity
Service->>DB: Increment user.campaignCount
Service-->>Controller: Campaign entity
Controller-->>Client: 201 {campaignId, transactionHash,<br/>targetAmount, donationToken, createdAt}
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
@chizzyedoka 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! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/migrations/CreateCoreEntities.js (2)
108-127:⚠️ Potential issue | 🟡 MinorDown migration may fail if wallet table pre-existed.
The
down()function drops wallet columns unconditionally, but if the wallet table existed before this migration ran (due toIF NOT EXISTS), dropping these columns may either fail or incorrectly remove columns that weren't added by this migration. Consider addingIF EXISTSchecks or documenting the expected state.🛡️ Suggested defensive approach
// Remove wallet columns - await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN "balance"`) - await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN "chain_name"`) - await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN "chain_id"`) - await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN "network"`) + await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN IF EXISTS "balance"`) + await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN IF EXISTS "chain_name"`) + await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN IF EXISTS "chain_id"`) + await queryRunner.query(`ALTER TABLE "wallet" DROP COLUMN IF EXISTS "network"`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/CreateCoreEntities.js` around lines 108 - 127, The down migration in async down(queryRunner) unconditionally drops wallet columns which can fail if the "wallet" table or specific columns pre-existed; update the down() logic to check existence before dropping: use queryRunner.hasTable('wallet') and/or queryRunner.hasColumn('wallet', '<column_name>') for each of "balance","chain_name","chain_id","network" and only run the corresponding ALTER TABLE DROP COLUMN if the table/column exists (or use IF EXISTS in the DROP statements), keeping the rest of the teardown (index/table drops) unchanged.
99-105:⚠️ Potential issue | 🟡 MinorIndex creation is not idempotent and may fail on re-run.
While the table and column creation are now idempotent with
IF NOT EXISTS, the index creation statements will fail if the indexes already exist. Consider usingCREATE INDEX IF NOT EXISTS(PostgreSQL 9.5+) for consistency.🐛 Suggested fix for idempotent index creation
// Create indexes - await queryRunner.query(`CREATE INDEX "User_email_key" ON "User" ("email")`) - await queryRunner.query(`CREATE INDEX "Distribution_created_at_idx" ON "Distribution" ("created_at")`) - await queryRunner.query(`CREATE INDEX "Distribution_status_idx" ON "Distribution" ("status")`) - await queryRunner.query(`CREATE INDEX "Distribution_transaction_hash_idx" ON "Distribution" ("transaction_hash")`) - await queryRunner.query(`CREATE INDEX "Distribution_user_address_idx" ON "Distribution" ("user_address")`) - await queryRunner.query(`CREATE INDEX "Wallet_address_idx" ON "wallet" ("address")`) - await queryRunner.query(`CREATE INDEX "Wallet_network_idx" ON "wallet" ("network")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "User_email_key" ON "User" ("email")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Distribution_created_at_idx" ON "Distribution" ("created_at")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Distribution_status_idx" ON "Distribution" ("status")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Distribution_transaction_hash_idx" ON "Distribution" ("transaction_hash")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Distribution_user_address_idx" ON "Distribution" ("user_address")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Wallet_address_idx" ON "wallet" ("address")`) + await queryRunner.query(`CREATE INDEX IF NOT EXISTS "Wallet_network_idx" ON "wallet" ("network")`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/migrations/CreateCoreEntities.js` around lines 99 - 105, The index creation is not idempotent: change the raw SQL passed to queryRunner.query for the index statements (the calls that create "User_email_key", "Distribution_created_at_idx", "Distribution_status_idx", "Distribution_transaction_hash_idx", "Distribution_user_address_idx", "Wallet_address_idx", "Wallet_network_idx") to use CREATE INDEX IF NOT EXISTS ... so re-running the migration won't error; update each queryRunner.query invocation to use the IF NOT EXISTS variant (e.g. `CREATE INDEX IF NOT EXISTS "User_email_key" ON "User" ("email")`) preserving the exact index names used in the diff.README.md (1)
27-28:⚠️ Potential issue | 🟡 MinorOutdated reference to Jest in "Before You Begin" section.
Line 27 still lists Jest as a prerequisite, but the test infrastructure was changed to Node's built-in test runner with c8 (as documented later in lines 161-164). Consider removing or updating this reference.
📝 Suggested fix
- **ESLint**: Linting for maintaining code quality. -- **Jest**: Unit testing framework.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 27 - 28, Update the "Before You Begin" section by removing or replacing the outdated bullet "**Jest**: Unit testing framework." so it reflects the current test tooling; specifically, either delete that line or change it to mention "Node's built-in test runner and c8" to match the later documentation (lines referencing Node test runner/c8). Ensure the README's prerequisites and the "Before You Begin" bullet list stay consistent with the existing test setup described elsewhere.
🧹 Nitpick comments (6)
src/__tests__/retry.test.ts (1)
22-36: Add an edge-case test for invalidretries.Current tests cover happy path and exhaustion, but not invalid option handling (e.g., negative retries). A small regression test here would lock in the expected guard behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/retry.test.ts` around lines 22 - 36, Add a new edge-case test in src/__tests__/retry.test.ts that verifies withRetry rejects/throws when given an invalid retries value (e.g., retries: -1). Call withRetry with a simple async function (that would succeed if called) and options { retries: -1, minDelayMs: 1, factor: 1 } and assert that it immediately throws/rejects (use assert.throws or assert.rejects depending on whether withRetry validates synchronously or returns a rejected promise) to lock in the guard behavior for invalid retries; reference the withRetry function in the test and assert that the call does not attempt retries.src/components/v1/user/user.entity.ts (1)
16-17: Consider addingnullable: falseto match the migration constraint.The migration defines
campaign_countasNOT NULL, but the entity decorator doesn't explicitly specifynullable: false. While the default value prevents null values in practice, adding the constraint improves type safety and documentation.♻️ Suggested fix
- `@Column`("integer", { name: "campaign_count", default: 0 }) + `@Column`("integer", { name: "campaign_count", default: 0, nullable: false }) campaignCount: number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/user/user.entity.ts` around lines 16 - 17, The Column decorator for the campaignCount property should explicitly declare nullable: false to match the migration; update the `@Column`(...) on campaignCount in the User entity (user.entity.ts) to include nullable: false so the decorator reads `@Column`("integer", { name: "campaign_count", default: 0, nullable: false }) ensuring the entity constraint aligns with the migration and improves type safety and documentation.src/components/v1/distribution/distrubtion.routes.ts (1)
11-12: Consider adding authentication to the GET endpoint.The new
listDistributionsendpoint at GET/has no authentication middleware, while distribution data (user addresses, transaction hashes, amounts) may be sensitive. If this data should be protected, consider adding JWT authentication similar to what's used in the campaigns routes.♻️ Suggested change if authentication is needed
+import { jwtAuth } from "../../../appMiddlewares/jwtAuth.api" + const distributionRouter = new EnhancedRouter() -distributionRouter.get("/", listDistributions) +distributionRouter.get("/", jwtAuth, listDistributions) distributionRouter.post("/", policyMiddleware(createDistributionSchema), createDistribution)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/distribution/distrubtion.routes.ts` around lines 11 - 12, The GET handler distributionRouter.get("/", listDistributions) lacks authentication; add the same JWT auth middleware used by the campaigns routes before the controller so sensitive distribution data is protected. Locate distributionRouter and update the route to include the auth middleware (e.g., jwtAuthMiddleware) ahead of listDistributions (same position as policyMiddleware is used on POST), ensuring middleware order matches campaigns routes' pattern and that the auth middleware is imported where distributionRouter is defined.src/services/cairo/campaignFactory.mock.ts (1)
5-5: Accept theaddressparameter for interface compliance.The
CairoCampaignClientinterface definesassertContractAccessible: (_address: string) => Promise<void>, but the mock omits the parameter. While this works at runtime, explicitly accepting the parameter improves clarity and ensures TypeScript doesn't report structural mismatches in strict mode.♻️ Suggested fix
- assertContractAccessible: async () => {}, + assertContractAccessible: async (_address: string) => {},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/cairo/campaignFactory.mock.ts` at line 5, The mock implementation of assertContractAccessible must accept the address parameter to match the CairoCampaignClient interface; update the mock function assertContractAccessible to declare an unused parameter (e.g., _address: string) and return a resolved Promise<void> (async () => {}) so the signature is identical to assertContractAccessible: (_address: string) => Promise<void> and TypeScript structural checks pass.src/components/v1/campaign/campaign.routes.ts (1)
14-17: Consider the "unknown" fallback behavior.If both
req.auth?.userIdandreq.ipare undefined, all such requests share the same rate limit bucket under"unknown". This is unlikely in practice (sincerequireJwtAuthApiruns first and rejects unauthenticated requests), but thereq.ip ?? "unknown"fallback could be simplified to justreq.ipsince auth middleware guaranteesuserIdpresence on successful auth.♻️ Optional simplification
keyGenerator: (req) => { const r = req as IRequest - return r.auth?.userId ?? req.ip ?? "unknown" + return r.auth?.userId ?? req.ip! },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaign/campaign.routes.ts` around lines 14 - 17, The current keyGenerator in campaign.routes uses req.auth?.userId ?? req.ip ?? "unknown", which can collapse unrelated requests into a single "unknown" bucket; since requireJwtAuthApi guarantees authenticated requests with userId, change keyGenerator to return r.auth!.userId ?? req.ip (or simply r.auth!.userId) and remove the "unknown" literal; update the function reference keyGenerator and any type cast to IRequest to rely on the guaranteed userId from the auth middleware.src/components/v1/campaign/campaign.validation.ts (1)
25-26: Remove redundantcampaign_refrefine.After Line 24
.trim()and Line 25.length(5), Line 26 adds no extra validation and the error text is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaign/campaign.validation.ts` around lines 25 - 26, The .refine on campaign_ref is redundant and has a misleading message—remove the .refine((s) => s.trim().length === 5, "campaign_ref cannot be empty") line; rely on the existing .trim() and .length(5, "campaign_ref must be exactly 5 characters long") chain on campaign_ref to enforce the rule (or, if you intended to validate trimmed length, ensure you call .transform(s => s.trim()) before .length instead of using the refine).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 19-35: The Starknet implementation is excluded from coverage and
untested; update the coverage config and add tests to exercise the non-mock
path: remove or adjust the "exclude" entry that filters out the Starknet
implementation (reference to campaignFactory.starknet.ts) in the c8
configuration so src/services/cairo/campaignFactory.starknet.ts is included, and
add unit/integration tests that invoke createCairoCampaignClient() from
campaignFactory.client.ts with CAIRO_MOCK unset (or set up a test harness/mocked
Starknet provider) so the code path returning createStarknetCampaignClient() is
executed and covered; ensure tests assert expected behavior of
createStarknetCampaignClient() and any initialization side effects.
In `@src/appMiddlewares/jwtAuth.api.ts`:
- Around line 28-29: The jwt.verify call in jwtAuth.api.ts is missing an
explicit algorithms option, leaving the endpoint vulnerable to algorithm
confusion attacks; update the call to jwt.verify(token,
appConfigs.authConfig.jwtSecret, { algorithms: [/* allowed algos e.g. "HS256"
*/] }) (keeping the cast to JwtClaims) so only the intended signing algorithms
are accepted, and derive the allowed algorithm(s) from your authConfig if
available; adjust the try block around the JwtClaims decode to use this options
object.
In `@src/components/v1/campaign/campaign.controller.ts`:
- Around line 16-35: mapCreateCampaignError currently maps known business errors
but treats DB unique-constraint failures as 500; update mapCreateCampaignError
to detect Postgres unique-violation (SQLSTATE 23505) as a duplicate-campaign-ref
and return {status: 409, code: "DUPLICATE_CAMPAIGN_REF", message: "campaign_ref
already exists"}; specifically, in mapCreateCampaignError add a branch that
checks the TypeORM/driver error shape (e.g. inspect (error as
any).driverError?.code === "23505" or (error as any).code === "23505" depending
on how TypeORM surfaces errors in this project) and map that to the 409 response
so concurrent save race conditions from CampaignService.createCampaign are
handled correctly.
In `@src/components/v1/campaign/campaign.entity.ts`:
- Around line 34-37: The entity method ensureId() must not synthesize a local
campaignId; remove the UUID fallback and instead make ensureId() validate that
this.campaignId exists and throw an explicit error (with context) if missing so
DB inserts fail fast; reference the BeforeInsert-decorated method ensureId() and
the campaignId property, and ensure callers (e.g.,
CampaignService.createCampaign()) supply the on-chain identifier rather than
relying on a generated UUID.
In `@src/components/v1/campaign/campaign.service.ts`:
- Around line 57-86: Currently the code calls
this.cairoClient.createCampaign(...) (transactionHash) before writing durable
state; if DB/audit fails a retry can re-submit the on-chain call. Change to an
idempotent flow: first insert a pending campaign record via
this.campaignRepository.save(this.campaignRepository.create({...})) including a
unique request key (e.g., requestId or idempotencyKey) and status="pending", and
persist the audit entry for the request; then call
this.cairoClient.createCampaign(...) to get transactionHash; then update the
pending record to status="confirmed" and set transactionHash (or use an
outbox/transaction to atomically publish the on-chain result); ensure functions
involved are campaignRepository.create/save and auditRepository.save are updated
to include and persist the idempotency key and status so retries detect and skip
duplicate on-chain submissions.
- Around line 29-34: The current pre-check using
this.campaignRepository.findOne({ where: { campaignRef } }) is race-prone; keep
or remove the pre-check but ensure the write path (e.g., where you call
campaignRepository.save/create in the methods that use campaignRef) catches DB
unique-constraint errors and rethrows an Error with code
"DUPLICATE_CAMPAIGN_REF". Specifically, wrap the repository save/create calls in
a try/catch, detect the DB driver/ORM unique-constraint error (e.g., error
codes/messages from TypeORM/DB driver), and in that catch throw a new
Error("Duplicate campaign_ref") with (error as any).code =
"DUPLICATE_CAMPAIGN_REF" so concurrent inserts map to the same error; apply the
same change to the other block that currently does a pre-check (the block around
lines 63-72).
- Around line 88-91: The current read-modify-write using
this.userRepository.findOne(...) and then update({ id: userId }, {
campaignCount: nextCount }) can race and lose concurrent increments; replace
that pattern with an atomic increment using this.userRepository.increment(...)
targeting the 'campaignCount' column for the given userId and handle the case
where result.affected is falsy (log/info that the user was not found). Update
references around userRepository and campaignCount accordingly and remove the
manual nextCount calculation and update call.
In `@src/components/v1/campaign/campaign.validation.ts`:
- Around line 39-45: The toU256Parts export must validate its input before
converting: ensure the string parses to a BigInt (use BigInt(value) inside a
try/catch), assert the value is non-negative and <= maxU256 = (1n << 256n) - 1n,
and throw a clear error/invariant if not; keep the existing splitting logic
(lowMask, low, high) but perform these guards early in the toU256Parts function
so malformed calldata cannot be produced from invalid inputs.
In `@src/config/persistence/data-source.ts`:
- Around line 67-79: The resetDatabase helper should only initialize/destroy the
shared AppDataSource when it actually does the initialization and must always
release the QueryRunner; update resetDatabase to check
AppDataSource.isInitialized before calling AppDataSource.initialize (so you skip
initialize if already initialized), record a boolean like didInitialize=true
only when you call and await AppDataSource.initialize(), use
AppDataSource.createQueryRunner() and ensure you always call
queryRunner.release() in a finally block after clearDatabase(), and call
AppDataSource.destroy() in the outer finally only if didInitialize is true
(avoiding unconditional destroy that can throw or tear down a
globally-initialized datasource).
- Around line 61-63: The current config branch sets ssl.rejectUnauthorized:
false for appConfigs.isProd/isStaging which disables certificate verification;
change it to enable verification and supply a CA bundle: when appConfigs.isProd
or appConfigs.isStaging set ssl.rejectUnauthorized = true and set ssl.ca to the
CA file contents (read from an env var like DB_SSL_CA_PATH or DB_SSL_CA_PEM and
load via fs.readFileSync or process.env) so the Postgres client validates the
server certificate; update the object you build (the part that currently uses {
ssl: { rejectUnauthorized: false } } ) to instead reference the CA and enable
verification, and add a clear error/log message if loading the CA fails.
In `@src/migrations/CreateCampaignAndAudit1760000000001.js`:
- Around line 13-24: The migration and entity timestamp columns use TIMESTAMP(3)
(naive time); update the migration CREATE TABLE definitions for "campaigns" and
"audit_logs" to use TIMESTAMPTZ(3) for their created_at columns, and update the
corresponding entity decorators (`@CreateDateColumn` in campaign.entity.ts and
auditLog.entity.ts) to specify type: "timestamptz" so API serialization (e.g.,
toISOString() in campaign.controller.ts) preserves timezone-aware timestamps.
In `@src/services/cairo/campaignFactory.client.ts`:
- Around line 23-24: Normalize CAIRO_MOCK before checking so non-exact values
don't fall through: replace the current isMock assignment with one that reads
env.CAIRO_MOCK (or empty string), calls .trim().toLowerCase(), and then compares
against accepted true values (e.g., "true", "1", "yes"); keep the ternary return
using createMockCampaignClient() and createStarknetCampaignClient() unchanged so
the selection uses the normalized boolean.
In `@src/services/cairo/campaignFactory.starknet.ts`:
- Around line 16-17: The code currently falls back to using transactionHash as
campaignId when the CAMPAIGN_CREATED_EVENT_KEY or event parsing fails; change
the logic in the campaign creation flow (the block using
env.CAMPAIGN_CREATED_EVENT_KEY / eventKey and the function that extracts the
campaign id from the parsed event) to fail closed: if eventKey is missing or
parsing the campaign-created event does not yield a valid campaignId, do not
persist or return a transactionHash as the campaign identifier—instead log the
error/context and return null (or throw) so no invalid campaignId is stored;
ensure the check is applied where eventKey is read and where the parsed
event->campaignId extraction occurs and remove any code path that assigns
transactionHash as campaignId.
In `@src/services/cairo/retry.ts`:
- Around line 11-13: Validate and guard the retry options before using them:
ensure retries is a non-negative integer, minDelayMs is a positive number, and
factor is a number >= 1 (and coerce/round retries if needed); if any option is
invalid, throw a clear RangeError (e.g. "Invalid retry options: retries=...,
minDelayMs=..., factor=...") instead of falling through to the later throw
undefined. Locate the variables retries, minDelayMs, and factor in retry.ts,
perform the checks right after they are derived from options, and replace the
current undefined throw path with this explicit, descriptive error so callers
get actionable feedback.
---
Outside diff comments:
In `@README.md`:
- Around line 27-28: Update the "Before You Begin" section by removing or
replacing the outdated bullet "**Jest**: Unit testing framework." so it reflects
the current test tooling; specifically, either delete that line or change it to
mention "Node's built-in test runner and c8" to match the later documentation
(lines referencing Node test runner/c8). Ensure the README's prerequisites and
the "Before You Begin" bullet list stay consistent with the existing test setup
described elsewhere.
In `@src/migrations/CreateCoreEntities.js`:
- Around line 108-127: The down migration in async down(queryRunner)
unconditionally drops wallet columns which can fail if the "wallet" table or
specific columns pre-existed; update the down() logic to check existence before
dropping: use queryRunner.hasTable('wallet') and/or
queryRunner.hasColumn('wallet', '<column_name>') for each of
"balance","chain_name","chain_id","network" and only run the corresponding ALTER
TABLE DROP COLUMN if the table/column exists (or use IF EXISTS in the DROP
statements), keeping the rest of the teardown (index/table drops) unchanged.
- Around line 99-105: The index creation is not idempotent: change the raw SQL
passed to queryRunner.query for the index statements (the calls that create
"User_email_key", "Distribution_created_at_idx", "Distribution_status_idx",
"Distribution_transaction_hash_idx", "Distribution_user_address_idx",
"Wallet_address_idx", "Wallet_network_idx") to use CREATE INDEX IF NOT EXISTS
... so re-running the migration won't error; update each queryRunner.query
invocation to use the IF NOT EXISTS variant (e.g. `CREATE INDEX IF NOT EXISTS
"User_email_key" ON "User" ("email")`) preserving the exact index names used in
the diff.
---
Nitpick comments:
In `@src/__tests__/retry.test.ts`:
- Around line 22-36: Add a new edge-case test in src/__tests__/retry.test.ts
that verifies withRetry rejects/throws when given an invalid retries value
(e.g., retries: -1). Call withRetry with a simple async function (that would
succeed if called) and options { retries: -1, minDelayMs: 1, factor: 1 } and
assert that it immediately throws/rejects (use assert.throws or assert.rejects
depending on whether withRetry validates synchronously or returns a rejected
promise) to lock in the guard behavior for invalid retries; reference the
withRetry function in the test and assert that the call does not attempt
retries.
In `@src/components/v1/campaign/campaign.routes.ts`:
- Around line 14-17: The current keyGenerator in campaign.routes uses
req.auth?.userId ?? req.ip ?? "unknown", which can collapse unrelated requests
into a single "unknown" bucket; since requireJwtAuthApi guarantees authenticated
requests with userId, change keyGenerator to return r.auth!.userId ?? req.ip (or
simply r.auth!.userId) and remove the "unknown" literal; update the function
reference keyGenerator and any type cast to IRequest to rely on the guaranteed
userId from the auth middleware.
In `@src/components/v1/campaign/campaign.validation.ts`:
- Around line 25-26: The .refine on campaign_ref is redundant and has a
misleading message—remove the .refine((s) => s.trim().length === 5,
"campaign_ref cannot be empty") line; rely on the existing .trim() and
.length(5, "campaign_ref must be exactly 5 characters long") chain on
campaign_ref to enforce the rule (or, if you intended to validate trimmed
length, ensure you call .transform(s => s.trim()) before .length instead of
using the refine).
In `@src/components/v1/distribution/distrubtion.routes.ts`:
- Around line 11-12: The GET handler distributionRouter.get("/",
listDistributions) lacks authentication; add the same JWT auth middleware used
by the campaigns routes before the controller so sensitive distribution data is
protected. Locate distributionRouter and update the route to include the auth
middleware (e.g., jwtAuthMiddleware) ahead of listDistributions (same position
as policyMiddleware is used on POST), ensuring middleware order matches
campaigns routes' pattern and that the auth middleware is imported where
distributionRouter is defined.
In `@src/components/v1/user/user.entity.ts`:
- Around line 16-17: The Column decorator for the campaignCount property should
explicitly declare nullable: false to match the migration; update the
`@Column`(...) on campaignCount in the User entity (user.entity.ts) to include
nullable: false so the decorator reads `@Column`("integer", { name:
"campaign_count", default: 0, nullable: false }) ensuring the entity constraint
aligns with the migration and improves type safety and documentation.
In `@src/services/cairo/campaignFactory.mock.ts`:
- Line 5: The mock implementation of assertContractAccessible must accept the
address parameter to match the CairoCampaignClient interface; update the mock
function assertContractAccessible to declare an unused parameter (e.g.,
_address: string) and return a resolved Promise<void> (async () => {}) so the
signature is identical to assertContractAccessible: (_address: string) =>
Promise<void> and TypeScript structural checks pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13cec429-4bf5-409a-8a1d-a17f5950b660
⛔ Files ignored due to path filters (35)
combined.logis excluded by!**/*.logdist/appMiddlewares/index.jsis excluded by!**/dist/**dist/appMiddlewares/jwtAuth.api.jsis excluded by!**/dist/**dist/components/v1/Donation/donation.controller.jsis excluded by!**/dist/**dist/components/v1/Donation/donation.dto.jsis excluded by!**/dist/**dist/components/v1/Donation/donation.entity.jsis excluded by!**/dist/**dist/components/v1/Donation/donation.service.jsis excluded by!**/dist/**dist/components/v1/Donation/donation.validation.jsis excluded by!**/dist/**dist/components/v1/audit/auditLog.entity.jsis excluded by!**/dist/**dist/components/v1/campaign/campaign.controller.jsis excluded by!**/dist/**dist/components/v1/campaign/campaign.entity.jsis excluded by!**/dist/**dist/components/v1/campaign/campaign.routes.jsis excluded by!**/dist/**dist/components/v1/campaign/campaign.service.jsis excluded by!**/dist/**dist/components/v1/campaign/campaign.validation.jsis excluded by!**/dist/**dist/components/v1/distribution/distribution.controller.jsis excluded by!**/dist/**dist/components/v1/distribution/distribution.dto.jsis excluded by!**/dist/**dist/components/v1/distribution/distribution.service.jsis excluded by!**/dist/**dist/components/v1/distribution/distribution.validation.jsis excluded by!**/dist/**dist/components/v1/distribution/distrubtion.routes.jsis excluded by!**/dist/**dist/components/v1/platform/platformControllers/permission.controller.jsis excluded by!**/dist/**dist/components/v1/routes.api.v1.jsis excluded by!**/dist/**dist/components/v1/routes.v1.jsis excluded by!**/dist/**dist/components/v1/user/user.entity.jsis excluded by!**/dist/**dist/components/v1/wallet/wallet.entity.jsis excluded by!**/dist/**dist/config/persistence/data-source.jsis excluded by!**/dist/**dist/config/persistence/seeder.jsis excluded by!**/dist/**dist/index.jsis excluded by!**/dist/**dist/services/cairo/campaignFactory.client.jsis excluded by!**/dist/**dist/services/cairo/campaignFactory.mock.jsis excluded by!**/dist/**dist/services/cairo/campaignFactory.starknet.jsis excluded by!**/dist/**dist/services/cairo/retry.jsis excluded by!**/dist/**dist/utils/apiResponse.jsis excluded by!**/dist/**dist/utils/index.jsis excluded by!**/dist/**error.logis excluded by!**/*.logpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
.env.example.gitignoreCODEBASE_FINDINGS.mdREADME.mdeslint.config.mjspackage.jsonsrc/__tests__/cairo.client.mock.test.tssrc/__tests__/campaign.service.test.tssrc/__tests__/campaign.validation.test.tssrc/__tests__/retry.test.tssrc/appMiddlewares/index.tssrc/appMiddlewares/jwtAuth.api.tssrc/components/v1/Donation/donation.validation.tssrc/components/v1/audit/auditLog.entity.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/distribution/distribution.controller.tssrc/components/v1/distribution/distribution.service.tssrc/components/v1/distribution/distrubtion.routes.tssrc/components/v1/platform/platform.utils.tssrc/components/v1/platform/platformControllers/permission.controller.tssrc/components/v1/routes.api.v1.tssrc/components/v1/routes.v1.tssrc/components/v1/user/user.entity.tssrc/components/v1/wallet/wallet.entity.tssrc/config/persistence/data-source.tssrc/config/persistence/seeder.tssrc/index.tssrc/migrations/CreateCampaignAndAudit1760000000001.jssrc/migrations/CreateCoreEntities.jssrc/services/cairo/campaignFactory.client.tssrc/services/cairo/campaignFactory.mock.tssrc/services/cairo/campaignFactory.starknet.tssrc/services/cairo/retry.tssrc/types/global.tssrc/utils/apiResponse.tssrc/utils/index.tstsconfig.build.tsbuildinfo
💤 Files with no reviewable changes (3)
- src/components/v1/Donation/donation.validation.ts
- src/components/v1/routes.v1.ts
- src/components/v1/platform/platformControllers/permission.controller.ts
| "c8": { | ||
| "include": [ | ||
| "src/components/v1/campaign/**/*.ts", | ||
| "src/services/cairo/retry.ts", | ||
| "src/services/cairo/campaignFactory.client.ts", | ||
| "src/services/cairo/campaignFactory.mock.ts", | ||
| "src/appMiddlewares/jwtAuth.api.ts", | ||
| "src/utils/apiResponse.ts" | ||
| ], | ||
| "exclude": [ | ||
| "src/services/cairo/campaignFactory.starknet.ts", | ||
| "src/config/**", | ||
| "src/utils/logger.ts", | ||
| "src/**/entity.ts", | ||
| "src/types/**" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether Starknet implementation is excluded and whether tests reference it.
jq '.c8' package.json
rg -n -C2 --type=ts 'campaignFactory\.starknet' src/__tests__ srcRepository: Fundable-Protocol/Backend
Length of output: 926
🏁 Script executed:
# Search for test files that reference the Starknet implementation
rg -n 'campaignFactory\.starknet|createStarknetCampaignClient' src/__tests__ --type=tsRepository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check if there are any tests for the Starknet file itself
find . -name '*starknet*.test.ts' -o -name '*starknet*.spec.ts' 2>/dev/null | head -20Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Look for test setup or configuration that might explain coverage strategy
cat -n package.json | sed -n '1,40p'Repository: Fundable-Protocol/Backend
Length of output: 1703
🏁 Script executed:
# Check the actual content and purpose of the excluded Starknet file
head -30 src/services/cairo/campaignFactory.starknet.tsRepository: Fundable-Protocol/Backend
Length of output: 1199
🏁 Script executed:
# Check how the client factory that imports the starknet file is tested
rg -n 'campaignFactory\.client|createCampaignClient' src/__tests__ --type=ts -A5Repository: Fundable-Protocol/Backend
Length of output: 1018
🏁 Script executed:
# Look for test files related to cairo/campaign
fd '\.test\.ts$|\.spec\.ts$' src/__tests__Repository: Fundable-Protocol/Backend
Length of output: 217
🏁 Script executed:
# Check if there's any documentation or comments explaining why starknet is excluded
rg -B3 -A3 'campaignFactory\.starknet' package.json src/Repository: Fundable-Protocol/Backend
Length of output: 904
🏁 Script executed:
# Check what functions campaignFactory.client exports and how they're used
rg -n 'export.*createCampaignClient|CreateCampaignOnChainArgs' src/services/cairo/ --type=ts -B2 -A8Repository: Fundable-Protocol/Backend
Length of output: 1769
Starknet integration is excluded from coverage and completely untested.
The createCairoCampaignClient() factory function (line 24 of campaignFactory.client.ts) returns createStarknetCampaignClient() when CAIRO_MOCK is not set, yet the Starknet implementation is excluded from coverage. Existing tests only exercise the mock path (CAIRO_MOCK=true), leaving the on-chain integration with zero test coverage.
Suggested fix
"c8": {
"include": [
"src/components/v1/campaign/**/*.ts",
"src/services/cairo/retry.ts",
"src/services/cairo/campaignFactory.client.ts",
"src/services/cairo/campaignFactory.mock.ts",
+ "src/services/cairo/campaignFactory.starknet.ts",
"src/appMiddlewares/jwtAuth.api.ts",
"src/utils/apiResponse.ts"
],
"exclude": [
- "src/services/cairo/campaignFactory.starknet.ts",
"src/config/**",
"src/utils/logger.ts",
"src/**/entity.ts",
"src/types/**"
]
},📝 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.
| "c8": { | |
| "include": [ | |
| "src/components/v1/campaign/**/*.ts", | |
| "src/services/cairo/retry.ts", | |
| "src/services/cairo/campaignFactory.client.ts", | |
| "src/services/cairo/campaignFactory.mock.ts", | |
| "src/appMiddlewares/jwtAuth.api.ts", | |
| "src/utils/apiResponse.ts" | |
| ], | |
| "exclude": [ | |
| "src/services/cairo/campaignFactory.starknet.ts", | |
| "src/config/**", | |
| "src/utils/logger.ts", | |
| "src/**/entity.ts", | |
| "src/types/**" | |
| ] | |
| }, | |
| "c8": { | |
| "include": [ | |
| "src/components/v1/campaign/**/*.ts", | |
| "src/services/cairo/retry.ts", | |
| "src/services/cairo/campaignFactory.client.ts", | |
| "src/services/cairo/campaignFactory.mock.ts", | |
| "src/services/cairo/campaignFactory.starknet.ts", | |
| "src/appMiddlewares/jwtAuth.api.ts", | |
| "src/utils/apiResponse.ts" | |
| ], | |
| "exclude": [ | |
| "src/config/**", | |
| "src/utils/logger.ts", | |
| "src/**/entity.ts", | |
| "src/types/**" | |
| ] | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 19 - 35, The Starknet implementation is excluded
from coverage and untested; update the coverage config and add tests to exercise
the non-mock path: remove or adjust the "exclude" entry that filters out the
Starknet implementation (reference to campaignFactory.starknet.ts) in the c8
configuration so src/services/cairo/campaignFactory.starknet.ts is included, and
add unit/integration tests that invoke createCairoCampaignClient() from
campaignFactory.client.ts with CAIRO_MOCK unset (or set up a test harness/mocked
Starknet provider) so the code path returning createStarknetCampaignClient() is
executed and covered; ensure tests assert expected behavior of
createStarknetCampaignClient() and any initialization side effects.
| try { | ||
| const decoded = jwt.verify(token, appConfigs.authConfig.jwtSecret) as JwtClaims |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
jsonwebtoken algorithm confusion vulnerability mitigation
💡 Result:
The JWT algorithm confusion vulnerability (also called key confusion) allows attackers to forge tokens by changing the 'alg' header from asymmetric (e.g., RS256) to symmetric (e.g., HS256), using the server's public key as the HMAC secret. This works if verification trusts the unverified 'alg' claim without restrictions. For the jsonwebtoken npm library (Node.js), the primary mitigation is to always specify the allowed algorithms explicitly in jwt.verify: jwt.verify(token, publicKey, { algorithms: ['RS256'] }); Do not omit the algorithms option or use jwt.verify(token, publicKey) alone, as it may allow mismatches. Update to v9.0.0+ to disable default 'none' algorithm support, preventing related bypasses. Additional best practices: - Use only one algorithm family (symmetric or asymmetric). - Never trust the 'alg' from the token header. - For asymmetric keys, fetch public keys securely (e.g., from JWKS endpoint) and validate 'kid'. - Reject 'none' algorithm. - Keep libraries updated; jsonwebtoken is not vulnerable to alg confusion when configured correctly. This follows official jsonwebtoken docs and security advisories.
Citations:
- 1: https://portswigger.net/web-security/jwt/algorithm-confusion
- 2: https://medium.com/@instatunnel/jwt-algorithm-confusion-turning-rs256-tokens-into-hs256-disasters-db1923774873
- 3: GHSA-c2ff-88x2-x9pg
- 4: https://docs.datadoghq.com/security/default_rules/def-000-t6u/
- 5: https://pentesterlab.com/blog/jwt-algorithm-confusion-code-review-lessons
- 6: https://blogs.jsmon.sh/what-is-jwt-key-confusion-attack-ways-to-exploit-examples-and-impact/
- 7: GHSA-qwph-4952-7xr6
- 8: https://www.npmjs.com/package/jsonwebtoken
- 9: https://github.com/auth0/node-jsonwebtoken
- 10: https://auth0.com/docs/tokens/guides/validate-jwts
🏁 Script executed:
# Find the jwtAuth file to see the full context
find . -type f -name "*jwtAuth*" | head -20Repository: Fundable-Protocol/Backend
Length of output: 141
🏁 Script executed:
# Search for JWT signing code to determine which algorithm is used
rg "jwt\.sign|jwt\.verify" -A 5 -B 2 --type ts --type js | head -100Repository: Fundable-Protocol/Backend
Length of output: 587
🏁 Script executed:
# Check appConfigs to understand the jwtSecret structure
rg "jwtSecret|authConfig" -A 3 -B 1 --type ts --type js | head -80Repository: Fundable-Protocol/Backend
Length of output: 1882
🏁 Script executed:
# Look for where tokens are created/signed to determine the algorithm
rg "sign.*token|createToken|generateToken" -A 5 -B 2 --type ts --type js | head -100Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Search for where tokens are created - broader search
rg "\.sign\(" -B 3 -A 5 --type ts --type js | head -150Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Search in service or utility files that might handle token signing
fd -e ts -e js | xargs rg -l "sign\|token" | grep -i "auth\|jwt\|token" | head -20Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check if there are any other jwt.verify calls in the codebase
rg "jwt\.verify" --type ts --type js -B 2 -A 3Repository: Fundable-Protocol/Backend
Length of output: 448
🏁 Script executed:
# Read the full jwtAuth file to see complete context
cat -n src/appMiddlewares/jwtAuth.api.tsRepository: Fundable-Protocol/Backend
Length of output: 1897
Add explicit algorithm specification to jwt.verify() to prevent algorithm confusion attacks.
The current call doesn't restrict which algorithms are accepted. Without an explicit algorithms option, jsonwebtoken could accept any algorithm in the token header, allowing attackers to forge tokens or exploit algorithm confusion vulnerabilities (e.g., switching from intended algorithm to alg: "none").
🔒 Suggested fix
- const decoded = jwt.verify(token, appConfigs.authConfig.jwtSecret) as JwtClaims
+ const decoded = jwt.verify(token, appConfigs.authConfig.jwtSecret, {
+ algorithms: ["HS256"],
+ }) as JwtClaims📝 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.
| try { | |
| const decoded = jwt.verify(token, appConfigs.authConfig.jwtSecret) as JwtClaims | |
| try { | |
| const decoded = jwt.verify(token, appConfigs.authConfig.jwtSecret, { | |
| algorithms: ["HS256"], | |
| }) as JwtClaims |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/appMiddlewares/jwtAuth.api.ts` around lines 28 - 29, The jwt.verify call
in jwtAuth.api.ts is missing an explicit algorithms option, leaving the endpoint
vulnerable to algorithm confusion attacks; update the call to jwt.verify(token,
appConfigs.authConfig.jwtSecret, { algorithms: [/* allowed algos e.g. "HS256"
*/] }) (keeping the cast to JwtClaims) so only the intended signing algorithms
are accepted, and derive the allowed algorithm(s) from your authConfig if
available; adjust the try block around the JwtClaims decode to use this options
object.
| const mapCreateCampaignError = (error: unknown) => { | ||
| const code = typeof (error as any)?.code === "string" ? (error as any).code : "INTERNAL_ERROR" | ||
|
|
||
| switch (code) { | ||
| case "DUPLICATE_CAMPAIGN_REF": | ||
| return { status: 409, code, message: "campaign_ref already exists" } | ||
| case "INSUFFICIENT_BALANCE": | ||
| return { status: 400, code, message: "Insufficient wallet balance for transaction fees" } | ||
| case "WALLET_NOT_FOUND": | ||
| return { status: 400, code, message: "Wallet not found" } | ||
| case "MISSING_WALLET_ADDRESS": | ||
| return { status: 400, code, message: "Token does not include a wallet address claim" } | ||
| default: | ||
| return { | ||
| status: 500, | ||
| code, | ||
| message: error instanceof Error ? error.message : "Internal server error", | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the campaign-related files
find . -type f -name "campaign*.ts" | grep -E "(controller|service)" | head -20Repository: Fundable-Protocol/Backend
Length of output: 351
🏁 Script executed:
# Also search for database schema/migration files
find . -type f \( -name "*.ts" -o -name "*.sql" \) | grep -iE "(migration|schema)" | head -20Repository: Fundable-Protocol/Backend
Length of output: 104
🏁 Script executed:
# Check the current controller implementation
cat -n src/components/v1/campaign/campaign.controller.ts | head -50Repository: Fundable-Protocol/Backend
Length of output: 2414
🏁 Script executed:
# Check campaign service implementation
cat -n src/components/v1/campaign/campaign.service.tsRepository: Fundable-Protocol/Backend
Length of output: 3739
🏁 Script executed:
# Find and check campaign entity
find . -type f -name "*campaign*.entity.ts"Repository: Fundable-Protocol/Backend
Length of output: 116
🏁 Script executed:
# Search for TypeORM error handling patterns in codebase
rg "driverError" --type ts -A 2 -B 2Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check campaign entity schema
cat -n src/components/v1/campaign/campaign.entity.tsRepository: Fundable-Protocol/Backend
Length of output: 1453
🏁 Script executed:
# Search for how TypeORM unique constraint errors are handled elsewhere
rg "QueryFailedError\|unique\|23505" --type ts -B 3 -A 3Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Search more broadly for error handling patterns with QueryFailedError
rg "QueryFailedError\|catch.*error" --type ts -B 2 -A 5 | head -100Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check how the service errors are caught in the controller
cat -n src/components/v1/campaign/campaign.controller.ts | sed -n '37,70p'Repository: Fundable-Protocol/Backend
Length of output: 1417
🏁 Script executed:
# Look for any other unique constraint error handling in the codebase
rg "unique\|constraint\|23505\|driverError" --type ts -iRepository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check the full error handling in controller
cat -n src/components/v1/campaign/campaign.controller.ts | sed -n '37,100p'Repository: Fundable-Protocol/Backend
Length of output: 2384
🏁 Script executed:
# Search for catch block that uses mapCreateCampaignError
rg "mapCreateCampaignError" --type ts -B 5 -A 5Repository: Fundable-Protocol/Backend
Length of output: 2015
🏁 Script executed:
# Look for any TypeORM QueryFailedError imports or usage
rg "import.*QueryFailedError\|from.*typeorm" --type ts | head -20Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check test files to see how errors are tested
cat -n src/__tests__/campaign.service.test.ts | head -100Repository: Fundable-Protocol/Backend
Length of output: 4051
🏁 Script executed:
# Look for any example of error handling with driver or database errors
rg "error.*code\|error.*driver" --type ts -B 2 -A 2 | head -80Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check package.json to verify TypeORM version
cat package.json | grep -A 2 -B 2 "typeorm"Repository: Fundable-Protocol/Backend
Length of output: 681
🏁 Script executed:
# Check TypeORM documentation and look for QueryFailedError usage patterns
rg "QueryFailedError" --type tsRepository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Look for any error handling with constraint violations or unique errors
rg "constraint\|UNIQUE\|unique.*violation" --type ts -i -B 3 -A 3Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check the data-source configuration to understand the database setup
cat -n src/config/persistence/data-source.ts | head -60Repository: Fundable-Protocol/Backend
Length of output: 2581
Handle DB-level campaign_ref unique constraint violations as 409.
CampaignService.createCampaign() performs a read-before-write duplicate check, but a window exists between that check (line 29) and the database save (line 63) where concurrent requests can bypass the precheck and trigger the unique constraint (defined on campaign.entity.ts line 5). These failures currently return 500 from the default error path instead of the correct 409 duplicate-conflict response.
The fix requires catching the PostgreSQL unique violation error and mapping it to 409. However, the exact error structure and path to access the constraint violation code needs verification before implementation—the suggested driverError.code path should be confirmed against how TypeORM surfaces constraint violations in this codebase's configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/v1/campaign/campaign.controller.ts` around lines 16 - 35,
mapCreateCampaignError currently maps known business errors but treats DB
unique-constraint failures as 500; update mapCreateCampaignError to detect
Postgres unique-violation (SQLSTATE 23505) as a duplicate-campaign-ref and
return {status: 409, code: "DUPLICATE_CAMPAIGN_REF", message: "campaign_ref
already exists"}; specifically, in mapCreateCampaignError add a branch that
checks the TypeORM/driver error shape (e.g. inspect (error as
any).driverError?.code === "23505" or (error as any).code === "23505" depending
on how TypeORM surfaces errors in this project) and map that to the 409 response
so concurrent save race conditions from CampaignService.createCampaign are
handled correctly.
| @BeforeInsert() | ||
| ensureId() { | ||
| if (!this.campaignId) this.campaignId = uuid() | ||
| } |
There was a problem hiding this comment.
Don't synthesize a local campaignId fallback.
campaignId is the on-chain identifier returned by CampaignService.createCampaign(). If that value is ever missing, Line 36 quietly inserts a random UUID instead of failing, which leaves the row impossible to reconcile with the actual Cairo campaign.
Suggested change
`@BeforeInsert`()
ensureId() {
- if (!this.campaignId) this.campaignId = uuid()
+ if (!this.campaignId) {
+ throw new Error("campaignId must be populated from the Cairo createCampaign result")
+ }
}📝 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.
| @BeforeInsert() | |
| ensureId() { | |
| if (!this.campaignId) this.campaignId = uuid() | |
| } | |
| `@BeforeInsert`() | |
| ensureId() { | |
| if (!this.campaignId) { | |
| throw new Error("campaignId must be populated from the Cairo createCampaign result") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/v1/campaign/campaign.entity.ts` around lines 34 - 37, The
entity method ensureId() must not synthesize a local campaignId; remove the UUID
fallback and instead make ensureId() validate that this.campaignId exists and
throw an explicit error (with context) if missing so DB inserts fail fast;
reference the BeforeInsert-decorated method ensureId() and the campaignId
property, and ensure callers (e.g., CampaignService.createCampaign()) supply the
on-chain identifier rather than relying on a generated UUID.
| 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 | ||
| } |
There was a problem hiding this comment.
Duplicate check is race-prone; handle uniqueness at write-time too.
The Line 29 pre-check can pass concurrently for multiple requests. The durable guard is the DB unique constraint, so save conflicts should be mapped to DUPLICATE_CAMPAIGN_REF explicitly.
Proposed fix
- 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
- }
+ 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
+ }
@@
- const saved = await this.campaignRepository.save(
- this.campaignRepository.create({
- campaignId,
- userId,
- campaignRef,
- targetAmount,
- donationToken,
- transactionHash,
- })
- )
+ let saved
+ try {
+ saved = await this.campaignRepository.save(
+ this.campaignRepository.create({
+ campaignId,
+ userId,
+ campaignRef,
+ targetAmount,
+ donationToken,
+ transactionHash,
+ })
+ )
+ } catch (err: any) {
+ if (String(err?.message ?? "").includes("campaigns_campaign_ref_key")) {
+ const conflict = new Error("Duplicate campaign_ref")
+ ;(conflict as any).code = "DUPLICATE_CAMPAIGN_REF"
+ throw conflict
+ }
+ throw err
+ }Also applies to: 63-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/v1/campaign/campaign.service.ts` around lines 29 - 34, The
current pre-check using this.campaignRepository.findOne({ where: { campaignRef }
}) is race-prone; keep or remove the pre-check but ensure the write path (e.g.,
where you call campaignRepository.save/create in the methods that use
campaignRef) catches DB unique-constraint errors and rethrows an Error with code
"DUPLICATE_CAMPAIGN_REF". Specifically, wrap the repository save/create calls in
a try/catch, detect the DB driver/ORM unique-constraint error (e.g., error
codes/messages from TypeORM/DB driver), and in that catch throw a new
Error("Duplicate campaign_ref") with (error as any).code =
"DUPLICATE_CAMPAIGN_REF" so concurrent inserts map to the same error; apply the
same change to the other block that currently does a pre-check (the block around
lines 63-72).
| try { | ||
| await AppDataSource.initialize(); | ||
|
|
||
| const queryRunner = AppDataSource.createQueryRunner(); | ||
|
|
||
| logger.info('Dropping all tables...'); | ||
| await queryRunner.clearDatabase(); | ||
| logger.info('All tables dropped. Database reset successfully!'); | ||
| } catch (error) { | ||
| console.error('Error resetting the database:', error); | ||
| } finally { | ||
| await AppDataSource.destroy(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/config/persistence/data-source.ts | head -100Repository: Fundable-Protocol/Backend
Length of output: 3357
🏁 Script executed:
# Search for the resetDatabase function definition and surrounding context
rg -A 20 "resetDatabase" src/config/persistence/data-source.tsRepository: Fundable-Protocol/Backend
Length of output: 571
🏁 Script executed:
# Search for all usages of resetDatabase in the codebase
rg "resetDatabase" --type typescript --type javascriptRepository: Fundable-Protocol/Backend
Length of output: 103
🏁 Script executed:
# Check if AppDataSource is from TypeORM
rg -B 5 "AppDataSource" src/config/persistence/data-source.ts | head -30Repository: Fundable-Protocol/Backend
Length of output: 661
🏁 Script executed:
# Search for all usages of resetDatabase in the codebase
rg "resetDatabase" --type jsRepository: Fundable-Protocol/Backend
Length of output: 292
🏁 Script executed:
# Also search without file type restrictions
rg "resetDatabase"Repository: Fundable-Protocol/Backend
Length of output: 373
🏁 Script executed:
# Check the TypeORM DataSource API documentation by searching for its methods
# Let's look for any other usage of AppDataSource.isInitialized in the codebase
rg "isInitialized"Repository: Fundable-Protocol/Backend
Length of output: 464
🏁 Script executed:
# Let's also check for any test files that might use resetDatabase
fd -e test.ts -e test.js -e spec.ts -e spec.jsRepository: Fundable-Protocol/Backend
Length of output: 217
🏁 Script executed:
# Check where resetDatabase is imported/used
rg "import.*resetDatabase|from.*resetDatabase"Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check the test files to see if resetDatabase is used there
grep -r "resetDatabase" src/__tests__/Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Let's check the campaign.controller and distribution.controller to see how isInitialized is used
cat -n src/components/v1/campaign/campaign.controller.ts | head -50Repository: Fundable-Protocol/Backend
Length of output: 2414
🏁 Script executed:
# Check for queryRunner.release() usage in the codebase
rg "queryRunner\.release|release\(\)"Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Let's also check what methods are available on AppDataSource from TypeORM
# by looking at how it's used elsewhere
rg "AppDataSource\." src/config/persistence/data-source.tsRepository: Fundable-Protocol/Backend
Length of output: 212
🏁 Script executed:
# Check if resetDatabase is exported and used elsewhere by looking at all imports of the file
rg "from.*data-source|import.*data-source"Repository: Fundable-Protocol/Backend
Length of output: 2379
🏁 Script executed:
# Let's check package.json to see which version of TypeORM is being used
cat package.json | grep -A 5 -B 5 "typeorm"Repository: Fundable-Protocol/Backend
Length of output: 1060
🏁 Script executed:
# Search for how AppDataSource is initialized elsewhere in the application
rg "AppDataSource\.initialize"Repository: Fundable-Protocol/Backend
Length of output: 281
🏁 Script executed:
# Let's look at the main application entry point to understand initialization flow
fd "main\|index\|app\|server" -e ts -e js | grep -E "^src/(main|index|app|server)" | head -10Repository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
# Check the src/index.ts to see how AppDataSource.initialize() is called
cat -n src/index.ts | head -50Repository: Fundable-Protocol/Backend
Length of output: 1779
🏁 Script executed:
# Let's search for where resetDatabase might be called (checking scripts or utilities)
rg -l "ensure-db|reset"Repository: Fundable-Protocol/Backend
Length of output: 240
🏁 Script executed:
# Check the ensure-db script mentioned in package.json
cat src/scripts/ensure-db.tsRepository: Fundable-Protocol/Backend
Length of output: 125
🏁 Script executed:
# Let's look up TypeORM 0.3.20 documentation behavior
# First, let's check if there are any comments or documentation in the repo about this
rg "isInitialized|initialize\(\)|destroy\(\)" src/ -B 2 -A 2Repository: Fundable-Protocol/Backend
Length of output: 1753
🌐 Web query:
TypeORM DataSource initialize isInitialized destroy 0.3.20 behavior
💡 Result:
In TypeORM version 0.3.20, the DataSource class manages database connections with the following lifecycle behavior for its key methods: initialize, isInitialized, and destroy. - DataSource is created with new DataSource(options), where isInitialized is initially false. - initialize establishes the database connection/pool, builds metadata, and sets isInitialized to true. Calling it when already initialized throws CannotConnectAlreadyConnectedError. - isInitialized is a readonly boolean indicating if initialize has completed successfully (connection established and metadata built). - destroy closes all connections and resources, setting isInitialized to false, allowing reinitialization. This standard behavior has remained consistent since the DataSource introduction in 0.3.0, with no changes specific to 0.3.20. Note a known limitation: isInitialized may remain true for stale/broken connections after certain errors, not reflecting connection health (issue #11163). Example usage: const dataSource = new DataSource({ /* options */ }); console.log(dataSource.isInitialized); // false await dataSource.initialize; console.log(dataSource.isInitialized); // true // Use dataSource.manager, dataSource.getRepository, etc. await dataSource.destroy; console.log(dataSource.isInitialized); // false Official docs confirm: call initialize on app bootstrap, destroy on shutdown. For long-running servers, typically initialize once and never destroy.
Citations:
- 1: https://typeorm.io/docs/data-source/data-source/
- 2: https://github.com/typeorm/typeorm/blob/master/src/data-source/DataSource.ts
- 3: https://typeorm.io/docs/data-source/data-source-api
- 4: https://tessl.io/registry/tessl/npm-typeorm/0.3.0/docs/data-source.md
- 5: https://github.com/typeorm/typeorm/releases/tag/0.3.0
- 6: DataSource.isInitialized Returns True for Stale or Broken Connections in Serverless Environments typeorm/typeorm#11163
🌐 Web query:
TypeORM queryRunner release method documentation
💡 Result:
The release method on TypeORM's QueryRunner releases the database connection back to the connection pool after use. It is essential to call this method when the query runner is no longer needed to avoid connection leaks, especially in databases supporting connection pooling like PostgreSQL or MySQL. Key points from the documentation: - Create a query runner: const queryRunner = dataSource.createQueryRunner; - Optionally connect explicitly: await queryRunner.connect; - Perform operations using queryRunner.manager or queryRunner.query. - Always release: await queryRunner.release;. After release, the query runner cannot be used further. Example: const queryRunner = dataSource.createQueryRunner; await queryRunner.connect; const users = await queryRunner.manager.find(User); await queryRunner.release; // Returns connection to pool In transactions: await queryRunner.startTransaction; try { // operations await queryRunner.commitTransaction; } catch (err) { await queryRunner.rollbackTransaction; } finally { await queryRunner.release; } Modern TypeScript also supports explicit resource management with using: await using queryRunner = dataSource.createQueryRunner; // operations - auto-released Failure to release can lead to "too many clients" errors, as connections remain held outside the pool.
Citations:
- 1: https://typeorm.io/docs/query-runner
- 2: https://dev.typeorm.io/docs/query-runner/
- 3: QueryRunner transaction example - Connection release needed? typeorm/typeorm#2602
- 4: https://typeorm.io/docs/advanced-topics/transactions/
- 5: https://typeorm.io/docs/query-runner/
Only tear down the shared data source if this helper initialized it.
The current implementation has three issues:
- If
AppDataSource.initialize()fails before the connection is marked initialized, the unconditionaldestroy()in thefinallyblock can throw and mask the original error. - Since
AppDataSourceis a global singleton (initialized insrc/index.ts), callingresetDatabase()while the app is running will attempt to callinitialize()on an already-initialized datasource, which throwsCannotConnectAlreadyConnectedError. Thefinallyblock then destroys the global datasource, breaking the application. - The
queryRunneris never released, causing connection leaks.
Suggested change
export const resetDatabase = async () => {
+ const shouldDestroy = !AppDataSource.isInitialized;
try {
- await AppDataSource.initialize();
+ if (shouldDestroy) {
+ await AppDataSource.initialize();
+ }
const queryRunner = AppDataSource.createQueryRunner();
-
- logger.info('Dropping all tables...');
- await queryRunner.clearDatabase();
- logger.info('All tables dropped. Database reset successfully!');
+ try {
+ logger.info('Dropping all tables...');
+ await queryRunner.clearDatabase();
+ logger.info('All tables dropped. Database reset successfully!');
+ } finally {
+ await queryRunner.release();
+ }
} catch (error) {
console.error('Error resetting the database:', error);
} finally {
- await AppDataSource.destroy();
+ if (shouldDestroy && AppDataSource.isInitialized) {
+ await AppDataSource.destroy();
+ }
}
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/config/persistence/data-source.ts` around lines 67 - 79, The
resetDatabase helper should only initialize/destroy the shared AppDataSource
when it actually does the initialization and must always release the
QueryRunner; update resetDatabase to check AppDataSource.isInitialized before
calling AppDataSource.initialize (so you skip initialize if already
initialized), record a boolean like didInitialize=true only when you call and
await AppDataSource.initialize(), use AppDataSource.createQueryRunner() and
ensure you always call queryRunner.release() in a finally block after
clearDatabase(), and call AppDataSource.destroy() in the outer finally only if
didInitialize is true (avoiding unconditional destroy that can throw or tear
down a globally-initialized datasource).
| CREATE TABLE IF NOT EXISTS "campaigns" ( | ||
| "campaign_id" text NOT NULL, | ||
| "user_id" text NOT NULL, | ||
| "campaign_ref" text NOT NULL, | ||
| "target_amount" numeric(78,0) NOT NULL, | ||
| "donation_token" text NOT NULL, | ||
| "transaction_hash" text NOT NULL, | ||
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| CONSTRAINT "PK_campaigns_campaign_id" PRIMARY KEY ("campaign_id"), | ||
| CONSTRAINT "UQ_campaigns_campaign_ref" UNIQUE ("campaign_ref") | ||
| ) | ||
| `) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "CreateCampaignAndAudit1760000000001.js" -type fRepository: Fundable-Protocol/Backend
Length of output: 124
🏁 Script executed:
cat -n src/migrations/CreateCampaignAndAudit1760000000001.js 2>/dev/null || echo "File not found, searching for migration files..."
fd -e js "migrations" --type f | head -20Repository: Fundable-Protocol/Backend
Length of output: 2673
🏁 Script executed:
rg -A 30 "campaigns.*text NOT NULL" --type jsRepository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
find . -name "*.ts" -path "*/entities/*" -type f | grep -i campaignRepository: Fundable-Protocol/Backend
Length of output: 51
🏁 Script executed:
rg -l "@CreateDateColumn" --type ts | head -10Repository: Fundable-Protocol/Backend
Length of output: 513
🏁 Script executed:
find . -path "*/campaign*" -name "*.ts" -type f | head -20Repository: Fundable-Protocol/Backend
Length of output: 543
🏁 Script executed:
rg "toISOString\(\)" --type ts -B 3 -A 3 | head -40Repository: Fundable-Protocol/Backend
Length of output: 631
🏁 Script executed:
cat -n src/components/v1/campaign/campaign.entity.tsRepository: Fundable-Protocol/Backend
Length of output: 1453
🏁 Script executed:
cat -n src/components/v1/audit/auditLog.entity.tsRepository: Fundable-Protocol/Backend
Length of output: 1319
🏁 Script executed:
cat -n src/components/v1/campaign/campaign.controller.ts | head -100Repository: Fundable-Protocol/Backend
Length of output: 4134
🏁 Script executed:
rg "TZ|timezone|TIMEZONE|process.env.TZ" --type ts --type js | head -20Repository: Fundable-Protocol/Backend
Length of output: 110
🏁 Script executed:
rg -i "timestamp" src/components/v1/campaign/campaign.service.ts src/components/v1/audit/auditLog.entity.ts -B 2 -A 2Repository: Fundable-Protocol/Backend
Length of output: 519
Use TIMESTAMPTZ for API-facing timestamp columns.
Both campaigns.created_at and audit_logs.created_at use TIMESTAMP(3), which stores naive local time without timezone information. When these values are serialized via toISOString() (line 73 in campaign.controller.ts), they can be misinterpreted if the database and application server operate in different timezones. Use TIMESTAMPTZ(3) instead and update the matching @CreateDateColumn definitions in both entity files to type: "timestamptz".
Changes needed
Migration (lines 20 and 38):
- "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ "created_at" TIMESTAMPTZ(3) NOT NULL DEFAULT CURRENT_TIMESTAMP,Entity decorators (campaign.entity.ts and auditLog.entity.ts):
- type: "timestamp",
+ type: "timestamptz",📝 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.
| CREATE TABLE IF NOT EXISTS "campaigns" ( | |
| "campaign_id" text NOT NULL, | |
| "user_id" text NOT NULL, | |
| "campaign_ref" text NOT NULL, | |
| "target_amount" numeric(78,0) NOT NULL, | |
| "donation_token" text NOT NULL, | |
| "transaction_hash" text NOT NULL, | |
| "created_at" TIMESTAMP(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| CONSTRAINT "PK_campaigns_campaign_id" PRIMARY KEY ("campaign_id"), | |
| CONSTRAINT "UQ_campaigns_campaign_ref" UNIQUE ("campaign_ref") | |
| ) | |
| `) | |
| CREATE TABLE IF NOT EXISTS "campaigns" ( | |
| "campaign_id" text NOT NULL, | |
| "user_id" text NOT NULL, | |
| "campaign_ref" text NOT NULL, | |
| "target_amount" numeric(78,0) NOT NULL, | |
| "donation_token" text NOT NULL, | |
| "transaction_hash" text NOT NULL, | |
| "created_at" TIMESTAMPTZ(3) NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| CONSTRAINT "PK_campaigns_campaign_id" PRIMARY KEY ("campaign_id"), | |
| CONSTRAINT "UQ_campaigns_campaign_ref" UNIQUE ("campaign_ref") | |
| ) | |
| `) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/migrations/CreateCampaignAndAudit1760000000001.js` around lines 13 - 24,
The migration and entity timestamp columns use TIMESTAMP(3) (naive time); update
the migration CREATE TABLE definitions for "campaigns" and "audit_logs" to use
TIMESTAMPTZ(3) for their created_at columns, and update the corresponding entity
decorators (`@CreateDateColumn` in campaign.entity.ts and auditLog.entity.ts) to
specify type: "timestamptz" so API serialization (e.g., toISOString() in
campaign.controller.ts) preserves timezone-aware timestamps.
| const isMock = env.CAIRO_MOCK === "true" || env.CAIRO_MOCK === "1" | ||
| return isMock ? createMockCampaignClient() : createStarknetCampaignClient() |
There was a problem hiding this comment.
Normalize CAIRO_MOCK parsing to avoid accidental Starknet execution.
Line 23 currently only matches exact literals. Values like "TRUE" or " true " will silently fall through to the real client.
Proposed fix
export const createCairoCampaignClient = (): CairoCampaignClient => {
- const isMock = env.CAIRO_MOCK === "true" || env.CAIRO_MOCK === "1"
+ const mockFlag = (env.CAIRO_MOCK ?? "").trim().toLowerCase()
+ const isMock = mockFlag === "true" || mockFlag === "1"
return isMock ? createMockCampaignClient() : createStarknetCampaignClient()
}📝 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.
| const isMock = env.CAIRO_MOCK === "true" || env.CAIRO_MOCK === "1" | |
| return isMock ? createMockCampaignClient() : createStarknetCampaignClient() | |
| export const createCairoCampaignClient = (): CairoCampaignClient => { | |
| const mockFlag = (env.CAIRO_MOCK ?? "").trim().toLowerCase() | |
| const isMock = mockFlag === "true" || mockFlag === "1" | |
| return isMock ? createMockCampaignClient() : createStarknetCampaignClient() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/cairo/campaignFactory.client.ts` around lines 23 - 24, Normalize
CAIRO_MOCK before checking so non-exact values don't fall through: replace the
current isMock assignment with one that reads env.CAIRO_MOCK (or empty string),
calls .trim().toLowerCase(), and then compares against accepted true values
(e.g., "true", "1", "yes"); keep the ternary return using
createMockCampaignClient() and createStarknetCampaignClient() unchanged so the
selection uses the normalized boolean.
| const eventKey = env.CAMPAIGN_CREATED_EVENT_KEY | ||
| if (!eventKey) return null |
There was a problem hiding this comment.
Fail closed when campaign-created event parsing fails; don’t fallback to tx hash as campaignId.
Line 71 currently masks event/misconfiguration failures by persisting transactionHash as campaignId. Since campaignId is a persisted identifier, this creates silent data-integrity drift.
Proposed fix
const parseEventCampaignId = (receipt: any): string | null => {
const eventKey = env.CAMPAIGN_CREATED_EVENT_KEY
- if (!eventKey) return null
+ if (!eventKey) {
+ throw new Error("Missing required env var: CAMPAIGN_CREATED_EVENT_KEY")
+ }
const normalizedKey = eventKey.toLowerCase()
const events: any[] = receipt?.events ?? []
const match = events.find((e) => (e.keys ?? []).map((k: string) => k.toLowerCase()).includes(normalizedKey))
if (!match) return null
@@
- const campaignId = parseEventCampaignId(receipt) ?? transactionHash
+ const campaignId = parseEventCampaignId(receipt)
+ if (!campaignId) {
+ throw new Error("create_campaign succeeded but CAMPAIGN_CREATED event was not found")
+ }
return { transactionHash, campaignId }Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/cairo/campaignFactory.starknet.ts` around lines 16 - 17, The
code currently falls back to using transactionHash as campaignId when the
CAMPAIGN_CREATED_EVENT_KEY or event parsing fails; change the logic in the
campaign creation flow (the block using env.CAMPAIGN_CREATED_EVENT_KEY /
eventKey and the function that extracts the campaign id from the parsed event)
to fail closed: if eventKey is missing or parsing the campaign-created event
does not yield a valid campaignId, do not persist or return a transactionHash as
the campaign identifier—instead log the error/context and return null (or throw)
so no invalid campaignId is stored; ensure the check is applied where eventKey
is read and where the parsed event->campaignId extraction occurs and remove any
code path that assigns transactionHash as campaignId.
| const retries = options?.retries ?? 3 | ||
| const minDelayMs = options?.minDelayMs ?? 250 | ||
| const factor = options?.factor ?? 2 |
There was a problem hiding this comment.
Guard invalid retry options to avoid throw undefined.
If retries is negative, the loop at Line 17 is skipped and Line 31 throws undefined, which loses actionable error context.
🔧 Proposed hardening
const retries = options?.retries ?? 3
const minDelayMs = options?.minDelayMs ?? 250
const factor = options?.factor ?? 2
+ if (!Number.isInteger(retries) || retries < 0) {
+ throw new RangeError("retries must be a non-negative integer")
+ }
@@
- throw lastError
+ throw lastError ?? new Error("withRetry failed without capturing an error")Also applies to: 31-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/cairo/retry.ts` around lines 11 - 13, Validate and guard the
retry options before using them: ensure retries is a non-negative integer,
minDelayMs is a positive number, and factor is a number >= 1 (and coerce/round
retries if needed); if any option is invalid, throw a clear RangeError (e.g.
"Invalid retry options: retries=..., minDelayMs=..., factor=...") instead of
falling through to the later throw undefined. Locate the variables retries,
minDelayMs, and factor in retry.ts, perform the checks right after they are
derived from options, and replace the current undefined throw path with this
explicit, descriptive error so callers get actionable feedback.
Closes #7
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation