Feat/backend api fundraising campaign endpoint#14
Open
laryhills wants to merge 4 commits intoFundable-Protocol:devfrom
Open
Feat/backend api fundraising campaign endpoint#14laryhills wants to merge 4 commits intoFundable-Protocol:devfrom
laryhills wants to merge 4 commits intoFundable-Protocol:devfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a complete campaign-creation feature: JWT auth and request rate limiting, input validation, StarkNet on-chain create_campaign with retry and event parsing, DB entity/service and persistence, utility helpers, config updates, and new runtime dependencies; startup now fails if JWT_SECRET is unset. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as API (Express)
participant Auth as Auth Middleware
participant Rate as Rate Limit (Redis)
participant Validator as Validator (Zod)
participant DB as Database
participant Balance as Balance Checker
participant StarkNet as StarkNet (RPC / Contract)
Client->>API: POST /api/v1/campaigns {campaign_ref, target_amount, donation_token}
API->>Auth: authMiddleware (verify JWT using secret, issuer, audience)
Auth-->>API: attach req.user
API->>Rate: campaignRateLimit (key: user.id or IP)
Rate-->>API: allow / reject (429)
API->>Validator: validate payload (createCampaignSchema)
Validator-->>API: validated input
API->>DB: isCampaignRefUnique(campaign_ref)
DB-->>API: unique? (yes/no)
API->>Balance: getUserWalletBalance(userWallet, donation_token)
Balance->>StarkNet: call balanceOf
StarkNet-->>Balance: balance
Balance-->>API: sufficient? (yes/no)
API->>StarkNet: verifyTokenContract(donation_token)
StarkNet-->>API: valid? (true/false)
API->>StarkNet: createCampaignOnChain(...) (with retries)
StarkNet-->>API: {campaign_id, transaction_hash}
API->>DB: saveCampaign({...})
DB-->>API: persisted campaign
API-->>Client: 201 Created {campaign_id, campaign_ref, target_amount, donation_token, transaction_hash, created_at}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: 16
🧹 Nitpick comments (2)
src/components/v1/campaigns/campaign.entity.ts (1)
15-19: Entity nullable columns vs.CampaignDatatype mismatch.
donation_tokenandtransaction_hashare markednullable: truein the entity, but theCampaignDatainterface insrc/types/campaign.tstypes them asstring(non-nullable). While the controller currently always provides values, this inconsistency could lead to subtle bugs if the interface is trusted elsewhere. Consider aligning the types — either make themstring | nullinCampaignDataor removenullable: trueif they are truly always required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaigns/campaign.entity.ts` around lines 15 - 19, The entity marks donation_token and transaction_hash as nullable (columns donation_token and transaction_hash in the Campaign entity) but the CampaignData type declares them as non-nullable strings; update the types to match by changing the CampaignData interface fields donation_token and transaction_hash to string | null (or, alternatively, remove nullable: true from the `@Column` decorators if they are truly required), ensuring consistency between the Campaign entity and the CampaignData type.src/utils/starknetService.ts (1)
56-56: Consider moving receipt confirmation out of the synchronous API path.Line 56 waits for on-chain confirmation inline. Returning
transaction_hashimmediately and confirming asynchronously will reduce request timeout risk and improve API responsiveness under network congestion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/starknetService.ts` at line 56, The synchronous call to provider.waitForTransaction(txHash) is blocking the API; modify the function that currently awaits provider.waitForTransaction (search for provider.waitForTransaction and the surrounding function that returns the transaction result) to return the transaction_hash immediately instead of awaiting confirmation, and kick off an independent background task/promise to wait for provider.waitForTransaction(txHash) and then handle post-confirmation work (logging, updating DB/state, retries or error handling) so the API response is fast while confirmation happens asynchronously.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 33-35: Replace the mainnet RPC default in .env.example by removing
the live URL and using a safe placeholder or testnet endpoint so local_dev can't
accidentally target mainnet; update the STARKNET_RPC_URL value to a clearly
labeled placeholder (e.g., STARKNET_RPC_URL=YOUR_STARKNET_RPC_URL or a testnet
URL) and consider adding a short comment above STARKNET_RPC_URL warning not to
use mainnet for local development—ensure the CAMPAIGN_CONTRACT_ADDRESS remains
empty as an explicit placeholder.
In `@src/appMiddlewares/auth.middleware.ts`:
- Around line 38-44: The jwt.verify call in auth.middleware.ts currently uses
jwt.verify(token, jwtSecret) without verification options; locate where tokens
are issued (search for jwt.sign or the external issuer) to determine the exact
algorithm, issuer and audience used, then update the jwt.verify call
(jwt.verify(token, jwtSecret, { algorithms: [...], issuer: '...', audience:
'...' })) to explicitly set algorithms, issuer and audience to match the token
generator; ensure you use the same algorithm string (e.g., HS256 or RS256) and
the exact issuer/audience values so authentication continues to work.
In `@src/appMiddlewares/campaignRateLimit.middleware.ts`:
- Around line 12-16: getLimiter currently caches initPromise permanently, so a
rejected createLimiter() leaves initPromise as a rejected promise and blocks
future retries; modify getLimiter to attach a rejection handler to the promise
returned by createLimiter() that clears/reset initPromise (e.g., set to
undefined/null) before rethrowing the error, so subsequent calls to getLimiter
will attempt to call createLimiter() again; reference the initPromise variable,
the getLimiter() function, and the createLimiter() call when making this change.
In `@src/components/v1/campaigns/campaign.controller.ts`:
- Around line 110-117: The call to saveCampaign can fail after the on-chain
campaign exists, leaving inconsistent state; wrap the saveCampaign(...) call in
its own try-catch inside the controller so failures are handled separately: on
catch, log the on-chain identifiers (campaign_id and transaction_hash), request
user id and any tx metadata for manual recovery, persist a recovery record or
emit an event for reconciliation, and return a 409 Conflict when the error is a
unique-constraint/duplicate-record (detect via the DB error code/message)
otherwise return/throw a 500 after logging; specifically update the controller
code that calls saveCampaign to catch DB unique-constraint errors and map them
to 409, while ensuring campaign_id/transaction_hash are persisted to logs/events
for manual repair.
- Around line 137-148: The catch block is returning error.message to clients
which leaks internal details; update the handler (the catch for createCampaign /
the function using logger.error) to send a generic client-facing message instead
(e.g., "Internal server error") in the res.status(500).json payload while
keeping the full error.message in logger.error; ensure the response's
error.message uses the generic string and that any error.details remains empty
or sanitized so only server logs (logger.error) contain the original error
variable/message.
- Around line 40-62: The donation token address is validated after calling
getUserWalletBalance; move the isValidContractAddress(donation_token) check
above the call to getUserWalletBalance so malformed addresses are rejected
early. Specifically, in the controller around getUserWalletBalance and
isValidContractAddress, first call isValidContractAddress(donation_token) and
return the INVALID_CONTRACT_ADDRESS 400 response if false, then proceed to call
getUserWalletBalance(req.user.walletAddress, donation_token) and perform the
INSUFFICIENT_BALANCE check.
In `@src/components/v1/campaigns/campaign.service.ts`:
- Around line 7-14: The TOCTOU gap between isCampaignRefUnique and saveCampaign
must be handled by catching unique-constraint DB errors during save and
translating them to a 409 conflict; update saveCampaign to wrap
campaignRepository.save(campaign) in a try/catch, detect the DB-specific
unique-violation (e.g., Postgres error code "23505" or the ORM's
UniqueConstraintViolation error type) and throw or return a domain-specific
error that the controller can convert to a 409 (or throw an
HttpConflict/ConflictError), leaving isCampaignRefUnique as a pre-check only;
reference saveCampaign and isCampaignRefUnique when making the change.
- Line 5: The module-level call to AppDataSource.getRepository(CampaignEntity)
(campaignRepository) creates a race where routes import the repository before
AppDataSource.initialize() finishes; replace this with a lazy accessor function
(e.g., getCampaignRepository) that checks AppDataSource.isInitialized (or awaits
AppDataSource.initialize() if appropriate) and then returns
AppDataSource.getRepository(CampaignEntity), and update all uses in
campaign.service.ts to call the lazy accessor instead of the module-level
campaignRepository; ensure the same pattern is applied to other services using
AppDataSource.getRepository.
In `@src/components/v1/campaigns/campaign.validation.ts`:
- Line 3: starknetAddressRegex in campaign.validation.ts is too permissive
(allows 1–64 hex chars); change it to require exactly 64 hex characters to match
the controller's isValidContractAddress rule: update the regex from
/^0x[0-9a-fA-F]{1,64}$/ to /^0x[0-9a-fA-F]{64}$/ and apply the same
exact-64-char change to any other occurrences in this file (the validation
schemas around the 22-31 region) so Zod validation fails fast with the same
constraint as isValidContractAddress in src/utils/helper.ts.
- Around line 25-29: The refine check computing feltMax (const feltMax = 2n **
251n + 17n * 2n ** 192n) in the validation refine closure currently uses a
strict less-than (BigInt(v) < feltMax) which excludes the intended maximum value
(P−1); update the comparison in that refine block to allow equality (use <=) so
values equal to feltMax are accepted—locate the refine closure that defines
feltMax and change the BigInt(v) comparison accordingly.
In `@src/index.ts`:
- Around line 102-105: The startup guard currently checks
appConfigs.authConfig.jwtSecret which can be a hardcoded fallback ('secret') so
it never fails; change the guard to validate the actual environment variable
instead (check process.env.JWT_SECRET) and/or reject known insecure defaults by
also failing when appConfigs.authConfig.jwtSecret === 'secret'; alternatively
remove the fallback in the config provider so jwtSecret is undefined when unset
and keep the existing guard—ensure the check references process.env.JWT_SECRET
or explicitly disallows the 'secret' literal to prevent startup with a
predictable key.
In `@src/utils/blockchainUtils.ts`:
- Around line 37-41: The catch block in src/utils/blockchainUtils.ts that
currently returns `{ value: 0n, lte: () => true }` is masking RPC/contract
failures as a valid zero balance; change it to surface errors instead of
pretending success — either rethrow the caught error (or throw a new Error with
context) or return an explicit error/result type (e.g., null or a { error }
object) so callers can distinguish infrastructure failures from genuine zero
balances; update any callers accordingly to handle the propagated error/result
rather than relying on the fake zero value.
In `@src/utils/helper.ts`:
- Around line 34-44: The u256FromString function currently validates format and
non-negativity but doesn't reject values > 2^256-1; update u256FromString to
compute the u256 maximum (const MAX_U256 = (1n << 256n) - 1n) and throw a
RangeError if the parsed BigInt (big) is greater than MAX_U256, keeping the
existing non-negative check and then computing low and high as before; reference
function name u256FromString and the variables big, low, high when making this
change.
In `@src/utils/starknetService.ts`:
- Around line 80-87: The code is throwing plain objects returned by
mapStarknetError (and possibly lastError) which breaks error semantics; update
places that currently do "throw mapStarknetError(...)" and the final throw of
lastError so they always throw an Error instance: call mapStarknetError, assign
its result to a variable, and if it's not an instanceof Error wrap it into a new
Error (e.g., new Error(err.message || 'Starknet error') and copy relevant
properties onto it) before throwing; apply the same pattern where lastError is
thrown so every thrown value is an Error instance.
- Around line 36-39: The code currently calls getUserPrivateKey(userWallet)
before validating userWallet and then always throws due to an unimplemented
resolver; reorder and implement: first check that userWallet is provided (throw
if missing) before calling getUserPrivateKey, then implement or stub a real
resolver inside getUserPrivateKey to return a usable privateKey (or throw a
descriptive error) and only then validate privateKey (throw with a clear message
if resolution fails). Update the logic around getUserPrivateKey, userWallet, and
privateKey in src/utils/starknetService.ts (ensure getUserPrivateKey returns
null/undefined only on explicit failure and that its errors include context so
callers can handle them).
- Around line 143-149: Remove the overly broad message.includes('0x') check from
the CAMPAIGN_REF_EMPTY mapping in src/utils/starknetService.ts: locate the block
that returns { code: 'CAMPAIGN_REF_EMPTY', ... } and delete the "||
message.includes('0x')" condition so only the explicit CAMPAIGN_REF_EMPTY signal
is matched (keep using the existing message.includes('CAMPAIGN_REF_EMPTY') check
and ensure the returned object still includes details: error); run/update any
tests that relied on the old broad matcher.
---
Nitpick comments:
In `@src/components/v1/campaigns/campaign.entity.ts`:
- Around line 15-19: The entity marks donation_token and transaction_hash as
nullable (columns donation_token and transaction_hash in the Campaign entity)
but the CampaignData type declares them as non-nullable strings; update the
types to match by changing the CampaignData interface fields donation_token and
transaction_hash to string | null (or, alternatively, remove nullable: true from
the `@Column` decorators if they are truly required), ensuring consistency between
the Campaign entity and the CampaignData type.
In `@src/utils/starknetService.ts`:
- Line 56: The synchronous call to provider.waitForTransaction(txHash) is
blocking the API; modify the function that currently awaits
provider.waitForTransaction (search for provider.waitForTransaction and the
surrounding function that returns the transaction result) to return the
transaction_hash immediately instead of awaiting confirmation, and kick off an
independent background task/promise to wait for
provider.waitForTransaction(txHash) and then handle post-confirmation work
(logging, updating DB/state, retries or error handling) so the API response is
fast while confirmation happens asynchronously.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/components/v1/routes.v1.jsis excluded by!**/dist/**dist/config/persistence/data-source.jsis excluded by!**/dist/**dist/utils/helper.jsis excluded by!**/dist/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.env.example.gitignorepackage.jsonsrc/appMiddlewares/auth.middleware.tssrc/appMiddlewares/campaignRateLimit.middleware.tssrc/components/v1/campaigns/campaign.controller.tssrc/components/v1/campaigns/campaign.entity.tssrc/components/v1/campaigns/campaign.routes.tssrc/components/v1/campaigns/campaign.service.tssrc/components/v1/campaigns/campaign.validation.tssrc/components/v1/routes.v1.tssrc/config/persistence/data-source.tssrc/index.tssrc/types/auth.tssrc/types/campaign.tssrc/types/express/index.d.tssrc/utils/blockchainUtils.tssrc/utils/campaign_donation.abi.jsonsrc/utils/helper.tssrc/utils/starknetService.tstsconfig.build.tsbuildinfo
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/appMiddlewares/auth.middleware.ts (1)
12-23:⚠️ Potential issue | 🟠 Major
!jwtSecretguard is unreachable dead code.Because
appConfigs.authConfig.jwtSecretdefaults to'secret'whenJWT_SECRETis unset, this check will never betrue. The 500 response intended to protect against misconfiguration is silently bypassed. The fix is at the root cause (src/config/index.ts, Line 25) — remove the|| 'secret'fallback so an absent secret properly triggers this guard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/appMiddlewares/auth.middleware.ts` around lines 12 - 23, The guard checking appConfigs.authConfig.jwtSecret is unreachable because the configuration code provides a default fallback ('secret'); remove the hardcoded fallback (the "|| 'secret'" default) from the config initialization so appConfigs.authConfig.jwtSecret can be undefined when JWT_SECRET is not set, then keep the existing guard in auth.middleware.ts (the !jwtSecret branch) which will now properly return the 500 response; update any tests or startup checks that relied on the fallback to assert a real secret is required.src/utils/blockchainUtils.ts (1)
37-41: Error propagation is now correct — past issue resolved.The catch block properly rethrows with context instead of masking RPC failures as a zero balance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/blockchainUtils.ts` around lines 37 - 41, The catch block in fetchWalletBalance already correctly rethrows an Error with context (using error.message when available) instead of swallowing RPC failures, so no code change is needed; keep the current throw in the catch block as written to preserve proper error propagation and logging context.
🧹 Nitpick comments (5)
src/components/v1/campaigns/campaign.service.ts (3)
32-44: TOCTOU graceful handling looks good — addresses the previousConflictErrormapping concern.Catching the DB-level unique constraint error and throwing a typed
ConflictErrorensures that concurrent duplicate inserts (which can pass theisCampaignRefUniquepre-check) still resolve to a clean 409, rather than a generic 500.One minor nit:
getRepo()is invoked twice (lines 33 and 35). Extracting it to a localconst repoavoids the double call and makes the intent clearer:♻️ Proposed refactor — single repo reference
export async function saveCampaign(data: CampaignData): Promise<CampaignEntity> { - const campaign = getRepo().create(data); + const repo = getRepo(); + const campaign = repo.create(data); try { - return await getRepo().save(campaign); + return await repo.save(campaign); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaigns/campaign.service.ts` around lines 32 - 44, Extract the repository into a local const to avoid calling getRepo() twice: inside saveCampaign, assign const repo = getRepo() and then use repo.create(data) and repo.save(campaign) instead of calling getRepo() again; keep the existing try/catch logic and error mapping (isUniqueConstraintViolation -> ConflictError) and ensure the function signature (saveCampaign, CampaignData, CampaignEntity) remains unchanged.
15-17: Consider using TypeORM's genericQueryFailedErrortyping instead of a manual cast.TypeORM 0.3.18 introduced
QueryFailedErrorgeneric typing fordriverError, soQueryFailedError<T>exposesreadonly driverError: T. The cast(err as { driverError?: { code?: string } })works but bypasses TypeORM's type system. A minor improvement is to use the generic directly:♻️ Proposed refactor — use TypeORM generic typing
- const code = (err as { driverError?: { code?: string } }).driverError?.code; + const code = (err as QueryFailedError<{ code?: string }>).driverError?.code;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaigns/campaign.service.ts` around lines 15 - 17, Replace the manual cast for extracting driver error code with TypeORM's generic QueryFailedError typing: where the code currently checks "if (err instanceof QueryFailedError) { const code = (err as { driverError?: { code?: string } }).driverError?.code; ... }", change the cast to use QueryFailedError<{ code?: string }> so you can access err.driverError.code via the typed QueryFailedError instance (keep the instanceof guard and the POSTGRES_UNIQUE_VIOLATION comparison). This touches the error-handling block in campaign.service.ts where QueryFailedError is inspected.
27-30: Preferexists()overcount()for a semantically clearer uniqueness check.TypeORM 0.3.11+ added
Repository.exists(), which is more direct and allows the DB to short-circuit on the first matching row rather than scanning the full set.♻️ Proposed refactor
export async function isCampaignRefUnique(campaign_ref: string): Promise<boolean> { - const count = await getRepo().count({ where: { campaign_ref } }); - return count === 0; + const exists = await getRepo().exists({ where: { campaign_ref } }); + return !exists; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/v1/campaigns/campaign.service.ts` around lines 27 - 30, Replace the current count-based uniqueness check in isCampaignRefUnique with Repository.exists to short-circuit on the first match: call getRepo().exists({ where: { campaign_ref } }) and return the negated result (i.e., true when no existing row). Update references in isCampaignRefUnique to remove the count() call and use exists() instead..env.example (1)
37-38: LGTM — testnet URL correctly replaces the previous mainnet default.Minor: the static analysis tool flags that
CAMPAIGN_CONTRACT_ADDRESS(Line 38) should be ordered beforeSTARKNET_RPC_URL(Line 37) alphabetically within the Starknet section.🔧 Proposed fix
# Starknet -STARKNET_RPC_URL=https://starknet-sepolia.public.blastapi.io/rpc/v0_8 CAMPAIGN_CONTRACT_ADDRESS= +STARKNET_RPC_URL=https://starknet-sepolia.public.blastapi.io/rpc/v0_8🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example around lines 37 - 38, Reorder the two Starknet environment variables so they are alphabetical: move CAMPAIGN_CONTRACT_ADDRESS to appear before STARKNET_RPC_URL in .env.example; ensure the variable names (CAMPAIGN_CONTRACT_ADDRESS and STARKNET_RPC_URL) remain unchanged and no other lines in the Starknet section are modified.src/appMiddlewares/auth.middleware.ts (1)
59-68: Silent catch discards all JWT error context — add logging.The empty
catchblock swallows the actual error (TokenExpiredError,JsonWebTokenError,NotBeforeError, etc.), making production authentication failures opaque to operators. At minimum, log the error at debug/warn level before returning the generic 401.♻️ Proposed fix
- } catch { + } catch (err) { + console.warn('[authMiddleware] JWT verification failed:', err instanceof Error ? err.message : err); res.status(401).json({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/appMiddlewares/auth.middleware.ts` around lines 59 - 68, The catch block in the auth middleware currently swallows JWT errors and returns a generic 401; update that catch to log the caught error (name, message, and stack or error object) at debug/warn level before sending res.status(401).json(...). Locate the catch in auth.middleware (the block that returns the UNAUTHORIZED JSON) and use the project logger instance (or console.warn if none) to record the error and any JWT-specific fields (e.g., TokenExpiredError) so operators can diagnose auth failures while still returning the same 401 response.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/appMiddlewares/campaignRateLimit.middleware.ts`:
- Around line 53-69: The async middleware campaignRateLimit currently awaits
getLimiter() without handling rejections, which can cause unhandled promise
rejections; wrap the await getLimiter() call in a try/catch, and if getLimiter()
throws call next(err) (or send a suitable 5xx response) and return to stop
execution; once you have a valid limiterInstance call limiterInstance(req, res,
next) as before. Reference getLimiter() and campaignRateLimit and ensure errors
from getLimiter() are propagated to Express via next(err) instead of letting the
promise reject.
In `@src/components/v1/campaigns/campaign.service.ts`:
- Around line 14-25: The fallback string-matching in isUniqueConstraintViolation
is too permissive and causes false-positive unique-constraint detections; remove
the broad `'already exists'` check (or remove the entire fallback and only rely
on the QueryFailedError branch that checks err.driverError.code ===
POSTGRES_UNIQUE_VIOLATION) so that only genuine Postgres unique-violation errors
(QueryFailedError / POSTGRES_UNIQUE_VIOLATION) return true; update
isUniqueConstraintViolation accordingly and keep references to QueryFailedError
and POSTGRES_UNIQUE_VIOLATION.
In `@src/config/index.ts`:
- Line 25: The config currently sets jwtSecret with a silent default ('secret')
which negates the env.JWT_SECRET check; remove the fallback (change jwtSecret to
use env.JWT_SECRET only) and add a fail-fast startup assertion that checks
authConfig.jwtSecret and throws if missing (so the guard in auth.middleware.ts
can be meaningful). Specifically: stop defaulting jwtSecret to 'secret' (use
env.JWT_SECRET directly) and add a one-time startup validation that throws a
clear error like "JWT_SECRET environment variable must be set" when
authConfig.jwtSecret is falsy.
In `@src/utils/blockchainUtils.ts`:
- Around line 44-53: The function verifyTokenContract currently swallows all
errors and returns false, conflating network/RPC failures with non-ERC20
contracts; update verifyTokenContract to catch errors from Contract(ERC20_ABI,
contractAddress, provider).symbol() specifically: detect network/RPC errors
(e.g., provider/network timeouts or RPC error codes) and either rethrow or
propagate a distinct error, and only return false for cases that clearly
indicate the contract lacks symbol() (e.g., ABI/method-not-found). Also add a
processLogger.error or console.error in the catch paths to record the original
error (include contractAddress and provider context) so transient infra issues
are visible; keep references to verifyTokenContract, Contract, symbol, and
provider when implementing the checks.
- Around line 3-6: The module currently creates a module-level RpcProvider using
RPC_URL (and a hardcoded Blast fallback) at import time, which prevents tests
from setting STARKNET_RPC_URL later and uses an unreliable public endpoint;
change this by removing the module-level instantiation of provider and RPC_URL,
require/accept a provider (or RPC URL) to be injected into exported functions
(or lazily construct RpcProvider inside functions like the ones that call
RpcProvider) so construction happens at call-time, and make the code fail fast
when STARKNET_RPC_URL is not provided (throw an error) instead of defaulting to
the Blast public endpoint; reference RpcProvider, RPC_URL, and any exported
functions in this file to locate where to inject or lazily create the provider.
- Around line 8-23: The ABI in ERC20_ABI uses Cairo 0 types and must be updated
to Cairo 1.x types: change balanceOf's input type 'felt' to 'ContractAddress'
and its output type from 'Uint256' to 'u256' (or appropriate Cairo 1 u256
alias), and change symbol's output type from 'felt' to 'ByteArray' (or Cairo 1
string/bytes type); update the entries for the functions named 'balanceOf' and
'symbol' inside ERC20_ABI accordingly so decoding and calls against Cairo 1.x
OpenZeppelin ERC20 contracts succeed.
---
Duplicate comments:
In `@src/appMiddlewares/auth.middleware.ts`:
- Around line 12-23: The guard checking appConfigs.authConfig.jwtSecret is
unreachable because the configuration code provides a default fallback
('secret'); remove the hardcoded fallback (the "|| 'secret'" default) from the
config initialization so appConfigs.authConfig.jwtSecret can be undefined when
JWT_SECRET is not set, then keep the existing guard in auth.middleware.ts (the
!jwtSecret branch) which will now properly return the 500 response; update any
tests or startup checks that relied on the fallback to assert a real secret is
required.
In `@src/utils/blockchainUtils.ts`:
- Around line 37-41: The catch block in fetchWalletBalance already correctly
rethrows an Error with context (using error.message when available) instead of
swallowing RPC failures, so no code change is needed; keep the current throw in
the catch block as written to preserve proper error propagation and logging
context.
---
Nitpick comments:
In @.env.example:
- Around line 37-38: Reorder the two Starknet environment variables so they are
alphabetical: move CAMPAIGN_CONTRACT_ADDRESS to appear before STARKNET_RPC_URL
in .env.example; ensure the variable names (CAMPAIGN_CONTRACT_ADDRESS and
STARKNET_RPC_URL) remain unchanged and no other lines in the Starknet section
are modified.
In `@src/appMiddlewares/auth.middleware.ts`:
- Around line 59-68: The catch block in the auth middleware currently swallows
JWT errors and returns a generic 401; update that catch to log the caught error
(name, message, and stack or error object) at debug/warn level before sending
res.status(401).json(...). Locate the catch in auth.middleware (the block that
returns the UNAUTHORIZED JSON) and use the project logger instance (or
console.warn if none) to record the error and any JWT-specific fields (e.g.,
TokenExpiredError) so operators can diagnose auth failures while still returning
the same 401 response.
In `@src/components/v1/campaigns/campaign.service.ts`:
- Around line 32-44: Extract the repository into a local const to avoid calling
getRepo() twice: inside saveCampaign, assign const repo = getRepo() and then use
repo.create(data) and repo.save(campaign) instead of calling getRepo() again;
keep the existing try/catch logic and error mapping (isUniqueConstraintViolation
-> ConflictError) and ensure the function signature (saveCampaign, CampaignData,
CampaignEntity) remains unchanged.
- Around line 15-17: Replace the manual cast for extracting driver error code
with TypeORM's generic QueryFailedError typing: where the code currently checks
"if (err instanceof QueryFailedError) { const code = (err as { driverError?: {
code?: string } }).driverError?.code; ... }", change the cast to use
QueryFailedError<{ code?: string }> so you can access err.driverError.code via
the typed QueryFailedError instance (keep the instanceof guard and the
POSTGRES_UNIQUE_VIOLATION comparison). This touches the error-handling block in
campaign.service.ts where QueryFailedError is inspected.
- Around line 27-30: Replace the current count-based uniqueness check in
isCampaignRefUnique with Repository.exists to short-circuit on the first match:
call getRepo().exists({ where: { campaign_ref } }) and return the negated result
(i.e., true when no existing row). Update references in isCampaignRefUnique to
remove the count() call and use exists() instead.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.env.examplesrc/appMiddlewares/auth.middleware.tssrc/appMiddlewares/campaignRateLimit.middleware.tssrc/components/v1/campaigns/campaign.controller.tssrc/components/v1/campaigns/campaign.service.tssrc/components/v1/campaigns/campaign.validation.tssrc/config/index.tssrc/index.tssrc/utils/blockchainUtils.tssrc/utils/helper.tssrc/utils/starknetService.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/index.ts
- src/components/v1/campaigns/campaign.validation.ts
- src/utils/helper.ts
- src/components/v1/campaigns/campaign.controller.ts
- src/utils/starknetService.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/appMiddlewares/campaignRateLimit.middleware.ts (1)
6-6: Consider strict Redis config outside local dev.Line 6 silently falls back to localhost when
REDIS_URLis missing, which can mask production misconfiguration. Consider fail-fast in non-development environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/appMiddlewares/campaignRateLimit.middleware.ts` at line 6, The constant REDIS_URL currently defaults to 'redis://localhost:6379' which can silently mask missing production config; change the initialization of REDIS_URL so it only falls back to localhost when NODE_ENV === 'development' (or a similar dev-only flag) and otherwise throws or exits early with a clear error when process.env.REDIS_URL is undefined; update references in campaignRateLimit.middleware (the REDIS_URL constant) to rely on this fail-fast behavior so production runs cannot accidentally use a local Redis.src/utils/blockchainUtils.ts (1)
105-107: Considerlogger.warninstead oflogger.errorfor the expected "contract lacks symbol" path.A missing
symbol()entrypoint is an expected business condition (the address is simply not an ERC20), not a system error. Logging it aterrorlevel will pollute error dashboards/alerting, especially if this function is called frequently during donation token validation.♻️ Proposed refactor
if (isContractLacksSymbolError(error)) { - logger.error('verifyTokenContract: contract lacks symbol()', logContext); + logger.warn('verifyTokenContract: contract lacks symbol()', logContext); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/blockchainUtils.ts` around lines 105 - 107, The current verifyTokenContract path treats an expected "contract lacks symbol()" result as a system error; change the logging level from logger.error to logger.warn in the branch that checks isContractLacksSymbolError(error) so this expected business condition (non-ERC20 address) doesn't trigger error alerts—keep the same logContext and message (or slightly reword to indicate expected non-ERC20) and return false as before; update any tests or callers that assert logging if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/appMiddlewares/campaignRateLimit.middleware.ts`:
- Around line 21-38: The limiter is created immediately after instantiating
redisClient in createLimiter, which can allow requests before Redis is ready;
modify createLimiter to explicitly wait for Redis readiness (e.g., await
redisClient.connect() or await a ping/ready event) before constructing limiter
and RedisStore, and set enableOfflineQueue: false on the Redis client to avoid
long offline queuing; on connection failure, log the error and either throw or
fall back to a safe in-memory/no-op limiter so limiter (and the RedisStore
sendCommand used by RedisStore) is never invoked while redisClient is not
connected.
In `@src/utils/blockchainUtils.ts`:
- Around line 34-36: The call to getProvider() is inside the try/catch for
fetching balances so provider configuration errors (e.g., missing
STARKNET_RPC_URL) are swallowed and rethrown as a balance fetch failure; move
the getProvider() invocation out of the try block (like verifyTokenContract
does) so configuration errors surface directly, then construct the Contract (new
Contract(ERC20_ABI, tokenAddress, provider)) and run
contract.balanceOf(walletAddress) inside the try; ensure you reference
getProvider(), Contract, and balanceOf when making the change.
- Around line 35-36: The code calls uint256.uint256ToBN(result.balance) but
result.balance from contract.balanceOf is already a bigint in starknet.js v7;
remove the uint256.uint256ToBN call and use result.balance directly (or convert
bigint to a BN only if your codebase truly needs a BN) — update the code paths
referencing uint256.uint256ToBN and change any downstream uses to accept a
bigint or explicitly convert with a bigint-to-BN helper instead.
---
Nitpick comments:
In `@src/appMiddlewares/campaignRateLimit.middleware.ts`:
- Line 6: The constant REDIS_URL currently defaults to 'redis://localhost:6379'
which can silently mask missing production config; change the initialization of
REDIS_URL so it only falls back to localhost when NODE_ENV === 'development' (or
a similar dev-only flag) and otherwise throws or exits early with a clear error
when process.env.REDIS_URL is undefined; update references in
campaignRateLimit.middleware (the REDIS_URL constant) to rely on this fail-fast
behavior so production runs cannot accidentally use a local Redis.
In `@src/utils/blockchainUtils.ts`:
- Around line 105-107: The current verifyTokenContract path treats an expected
"contract lacks symbol()" result as a system error; change the logging level
from logger.error to logger.warn in the branch that checks
isContractLacksSymbolError(error) so this expected business condition (non-ERC20
address) doesn't trigger error alerts—keep the same logContext and message (or
slightly reword to indicate expected non-ERC20) and return false as before;
update any tests or callers that assert logging if present.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/appMiddlewares/campaignRateLimit.middleware.tssrc/components/v1/campaigns/campaign.service.tssrc/config/index.tssrc/index.tssrc/utils/blockchainUtils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/v1/campaigns/campaign.service.ts
- src/config/index.ts
- src/index.ts
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
1 participant
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.
🚀 Backend API Endpoint for Fundraising Campaign Creation
Type of Change
Summary by CodeRabbit
New Features
Chores
Style