feat: update clientPayment status #1043
Conversation
lissavxo
force-pushed
the
feat/update-payment-id
branch
from
0edfea9 to
fa5727b
Compare
lissavxo
force-pushed
the
feat/update-payment-id
branch
from
ab9676e to
0714f9e
Compare
lissavxo
force-pushed
the
feat/update-payment-id
branch
2 times, most recently
from
a940c12 to
bc6a25a
Compare
WalkthroughAdds client payment support: new POST API to create paymentId; Prisma model, enum, and migration for ClientPayment; transactionService functions to generate/get/update client payments; chronikService updates to set client payment statuses on mempool/confirmed events; validator helpers; 405 constant rename and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant API as API /api/payments/paymentId
participant Validators as Validators
participant TxSvc as transactionService
participant DB as Prisma DB
Client->>API: POST { address, amount? }
API->>Validators: parseCreatePaymentIdPOSTRequest()
Validators-->>API: { address, amount? }
API->>TxSvc: generatePaymentId(address, amount?)
TxSvc->>DB: upsert Address, create ClientPayment(status=PENDING)
DB-->>TxSvc: client payment created
TxSvc-->>API: paymentId
API-->>Client: 200 { paymentId }
alt invalid address
API-->>Client: configured 4xx response
else server error
API-->>Client: 500 { statusCode:500, message }
end
sequenceDiagram
autonumber
participant WS as Chronik WS
participant Chronik as ChronikBlockchainClient
participant TxSvc as transactionService
participant DB as Prisma DB
WS-->>Chronik: TX_ADDED_TO_MEMPOOL / TX_CONFIRMED (tx, outputs, OP_RETURN)
Chronik->>Chronik: parse OP_RETURN -> paymentId?
alt paymentId found
Chronik->>TxSvc: getClientPayment(paymentId)
TxSvc->>DB: find ClientPayment + Address
DB-->>TxSvc: ClientPayment or null
alt Found and (amount matches or amount is null)
Chronik->>TxSvc: updateClientPaymentStatus(paymentId, status)
TxSvc->>DB: update status (ADDED_TO_MEMPOOL or CONFIRMED)
else Not found or amount mismatch
Chronik-->>Chronik: no-op
end
else No paymentId
Chronik-->>Chronik: no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)services/chronikService.ts (2)
services/transactionService.ts (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
services/chronikService.ts (1)
437-440: Good move: pre-fetching tx and related addresses once.This addresses the prior request to centralize context and reduces duplication across branches. The helper usage on confirmation is clean.
Also applies to: 454-454
🧹 Nitpick comments (7)
prisma-local/schema.prisma (2)
25-25: Rename relation field for clarity.
clientPaymentIdsholdsClientPayment[], not just IDs. Suggest renaming toclientPaymentsto match semantics and existing naming patterns.Apply:
- clientPaymentIds ClientPayment[] + clientPayments ClientPayment[]
279-281: Optional: tighten paymentId column type.If
paymentIdis always a 32-char hex (uuid without dashes), you can use@db.Char(32)to reduce storage and improve index density. Requires updating the migration too.pages/api/payments/paymentId/index.ts (2)
17-23: Return 400 for invalid address as well.Map
INVALID_ADDRESS_400to its status code for clearer client errors. If you adopt theamountvalidation above, mapINVALID_INPUT_400too.Apply:
switch (error.message) { case RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400.message: res.status(RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400.statusCode).json(RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400) break + case RESPONSE_MESSAGES.INVALID_ADDRESS_400.message: + res.status(RESPONSE_MESSAGES.INVALID_ADDRESS_400.statusCode).json(RESPONSE_MESSAGES.INVALID_ADDRESS_400) + break + case RESPONSE_MESSAGES.INVALID_INPUT_400.message: + res.status(RESPONSE_MESSAGES.INVALID_INPUT_400.statusCode).json(RESPONSE_MESSAGES.INVALID_INPUT_400) + break default: res.status(500).json({ statusCode: 500, message: error.message }) }
26-28: Use 405 for unsupported methods.
METHOD_NOT_ALLOWEDin constants uses 500; respond with 405 here to be accurate (and consider fixing the constant in a follow-up).Apply:
- res.status(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED.statusCode) - .json(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED) + res.status(405).json({ statusCode: 405, message: 'Method not allowed.' })prisma-local/migrations/20250904155444_client_payment/migration.sql (1)
12-14: Optional: add index for addressString.MySQL may auto-create an index for FK columns, but adding it explicitly is clearer and portable.
Apply:
) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; +-- Index to support FK lookups +CREATE INDEX `ClientPayment_addressString_idx` ON `ClientPayment`(`addressString`); -- AddForeignKey ALTER TABLE `ClientPayment` ADD CONSTRAINT `ClientPayment_addressString_fkey` FOREIGN KEY (`addressString`) REFERENCES `Address`(`address`) ON DELETE RESTRICT ON UPDATE CASCADE;services/chronikService.ts (1)
14-19: Optional: prefer enum member over string casting.If Prisma generated enums are available at runtime, use
ClientPaymentStatus.CONFIRMED/ADDED_TO_MEMPOOLto avoid typos.services/transactionService.ts (1)
12-13: Avoid circular dependency between services.
transactionServiceimportsmultiBlockchainClientfromchronikService, whilechronikServiceimports functions fromtransactionService. ExtractmultiBlockchainClientto a small module (e.g.,services/multiBlockchainClient.ts) and import it from both sides to break the cycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pages/api/payments/paymentId/index.ts(1 hunks)prisma-local/migrations/20250904155444_client_payment/migration.sql(1 hunks)prisma-local/migrations/migration_lock.toml(1 hunks)prisma-local/schema.prisma(2 hunks)services/chronikService.ts(4 hunks)services/transactionService.ts(3 hunks)utils/validators.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
pages/api/payments/paymentId/index.ts (3)
utils/validators.ts (2)
parseCreatePaymentIdPOSTRequest(575-587)parseAddress(31-48)services/transactionService.ts (1)
generatePaymentId(969-1002)constants/index.ts (1)
RESPONSE_MESSAGES(8-107)
utils/validators.ts (1)
constants/index.ts (1)
RESPONSE_MESSAGES(8-107)
services/transactionService.ts (3)
constants/index.ts (1)
NETWORK_IDS_FROM_SLUGS(131-134)services/addressService.ts (1)
addressExists(127-141)services/chronikService.ts (1)
multiBlockchainClient(886-886)
services/chronikService.ts (3)
types/chronikTypes.ts (1)
AddressWithTransaction(11-14)utils/validators.ts (1)
parseOpReturnData(433-461)services/transactionService.ts (2)
updatePaymentStatus(1004-1009)getClientPayment(1011-1016)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (5)
prisma-local/migrations/migration_lock.toml (1)
3-3: Correct Prisma provider entry.Good catch fixing the stray dash; Prisma will read the provider correctly now.
prisma-local/schema.prisma (1)
273-277: ClientPaymentStatus enum looks good.pages/api/payments/paymentId/index.ts (1)
7-8: Test plan mismatch (GET vs POST).The PR test plan mentions calling
api/payments/paymentId?address=...(suggesting GET), but this handler only accepts POST. Either update the test plan or add GET support.services/transactionService.ts (2)
1004-1009: LGTM: simple, idempotent status update.Works as expected; callers should already guard existence.
1011-1016: LGTM: explicit include of address for downstream checks.Returning the address relation enables safe validations at call sites.
| `addressString` VARCHAR(191) NOT NULL, | ||
| `amount` DECIMAL(65, 30) NULL, |
There was a problem hiding this comment.
Fix FK column length to match referenced column.
ClientPayment.addressString must match Address.address length (255). As written (191), the FK add will fail on MySQL.
Apply:
- `addressString` VARCHAR(191) NOT NULL,
+ `addressString` VARCHAR(255) NOT NULL,📝 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.
| `addressString` VARCHAR(191) NOT NULL, | |
| `amount` DECIMAL(65, 30) NULL, | |
| `addressString` VARCHAR(255) NOT NULL, | |
| `amount` DECIMAL(65, 30) NULL, |
🤖 Prompt for AI Agents
In prisma-local/migrations/20250904155444_client_payment/migration.sql around
lines 5 to 6, the foreign-key column `addressString` is defined as VARCHAR(191)
which does not match the referenced `Address.address` length (255) and will
break the FK creation; update the column definition to VARCHAR(255) NOT NULL so
its length and nullability match the referenced column (and re-run/apply the
migration or regenerate migration SQL accordingly).
prisma-local/schema.prisma
Outdated
| model ClientPayment { | ||
| paymentId String @id | ||
| status ClientPaymentStatus | ||
| addressString String | ||
| amount Decimal? | ||
| address Address @relation(fields: [addressString], references: [address]) | ||
| createdAt DateTime @default(now()) | ||
| updatedAt DateTime @updatedAt | ||
| } |
There was a problem hiding this comment.
Foreign key length mismatch will break migration.
ClientPayment.addressString references Address.address (@db.VarChar(255)), but here it has no column length (migration creates it as VARCHAR(191)). MySQL requires identical type/length/collation for FK columns; this will fail when adding the FK.
Apply:
model ClientPayment {
paymentId String @id
status ClientPaymentStatus
- addressString String
+ addressString String @db.VarChar(255)
amount Decimal?
address Address @relation(fields: [addressString], references: [address])
createdAt DateTime @default(now())
updatedAt DateTime @updatedAt
+ @@index([addressString], map: "ClientPayment_addressString_fkey")
}📝 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.
| model ClientPayment { | |
| paymentId String @id | |
| status ClientPaymentStatus | |
| addressString String | |
| amount Decimal? | |
| address Address @relation(fields: [addressString], references: [address]) | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| } | |
| model ClientPayment { | |
| paymentId String @id | |
| status ClientPaymentStatus | |
| addressString String @db.VarChar(255) | |
| amount Decimal? | |
| address Address @relation(fields: [addressString], references: [address]) | |
| createdAt DateTime @default(now()) | |
| updatedAt DateTime @updatedAt | |
| @@index([addressString], map: "ClientPayment_addressString_fkey") | |
| } |
🤖 Prompt for AI Agents
In prisma-local/schema.prisma around lines 279 to 287, the
ClientPayment.addressString FK column lacks the explicit VARCHAR length and will
be generated as VARCHAR(191), which mismatches Address.address
(@db.VarChar(255)) and will break the FK migration; fix by annotating
ClientPayment.addressString with the same database type/length (e.g., add
@db.VarChar(255)) and ensure any custom collation matches Address.address so
both columns have identical type/length/collation before running the migration.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
prisma-local/schema.prisma (1)
279-286: Fix FK length mismatch (+ align referential action and decimal precision).
ClientPayment.addressStringmust matchAddress.address(@db.VarChar(255)), or MySQL will reject the FK at migration time.- Explicitly set
onUpdate: Cascadeto match the migration.- Align
amountprecision withTransaction.amount(@db.Decimal(24, 8)) to avoid inconsistent scales.Apply:
model ClientPayment { paymentId String @id status ClientPaymentStatus - addressString String - amount Decimal? - address Address @relation(fields: [addressString], references: [address]) + addressString String @db.VarChar(255) + amount Decimal? @db.Decimal(24, 8) + address Address @relation(fields: [addressString], references: [address], onUpdate: Cascade) createdAt DateTime @default(now()) updatedAt DateTime @updatedAt + @@index([addressString], map: "ClientPayment_addressString_fkey") }prisma-local/migrations/20250912183436_client_payment/migration.sql (1)
3-6: Make FK column length consistent and index it; align amount scale.
- Use
VARCHAR(255)foraddressStringto matchAddress.addressand prevent FK creation failure.- Add an index on
addressString(good practice; some engines auto-create, but being explicit avoids surprises).- Use
DECIMAL(24, 8)foramountto match application semantics andTransaction.amount.Apply:
CREATE TABLE `ClientPayment` ( `paymentId` VARCHAR(191) NOT NULL, `status` ENUM('PENDING', 'ADDED_TO_MEMPOOL', 'CONFIRMED') NOT NULL, - `addressString` VARCHAR(191) NOT NULL, - `amount` DECIMAL(65, 30) NULL, + `addressString` VARCHAR(255) NOT NULL, + `amount` DECIMAL(24, 8) NULL, `createdAt` DATETIME(3) NOT NULL DEFAULT CURRENT_TIMESTAMP(3), `updatedAt` DATETIME(3) NOT NULL, PRIMARY KEY (`paymentId`) ) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci; +-- Ensure supporting index exists for FK (explicit) +CREATE INDEX `ClientPayment_addressString_fkey` ON `ClientPayment`(`addressString`); + -- AddForeignKey ALTER TABLE `ClientPayment` ADD CONSTRAINT `ClientPayment_addressString_fkey` FOREIGN KEY (`addressString`) REFERENCES `Address`(`address`) ON DELETE RESTRICT ON UPDATE CASCADE;Also applies to: 13-14
services/chronikService.ts (1)
424-436: Harden status updates: validate paymentId, compare Decimals correctly, guard downgrades, and verify address.Current code can throw on missing/unknown IDs, mis-compare decimals via Number(), and update mismatched addresses; it may also downgrade CONFIRMED back to ADDED_TO_MEMPOOL.
Apply:
- private async updateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { - const parsedOpReturn = parseOpReturnData(opReturn ?? '') - const paymentId = parsedOpReturn.paymentId - - const clientPayment = await getClientPayment(paymentId) - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount)) { - await updatePaymentStatus(paymentId, status) - } - } else { - await updatePaymentStatus(paymentId, status) - } - } + private async updateClientPaymentStatus (txAmount: Prisma.Decimal, opReturn: string | undefined, status: ClientPaymentStatus, addressString: string): Promise<void> { + const parsed = parseOpReturnData(opReturn ?? '') as any + const paymentId: string = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + // TODO: confirm required length; adjust regex if generatePaymentId uses a different size + if (!/^[0-9a-fA-F]{32}$/.test(paymentId)) return + try { + const clientPayment = await getClientPayment(paymentId) + // Prevent mismatched updates and status downgrades/no‑ops + if (clientPayment.address.address !== addressString) return + if (status === 'ADDED_TO_MEMPOOL' && clientPayment.status === 'CONFIRMED') return + if (clientPayment.status === status) return + if (clientPayment.amount !== null) { + if (clientPayment.amount.equals(new Prisma.Decimal(txAmount))) { + await updatePaymentStatus(paymentId, status) + } + } else { + await updatePaymentStatus(paymentId, status) + } + } catch { + // Unknown paymentId — ignore + } + }
🧹 Nitpick comments (2)
services/chronikService.ts (2)
33-33: Narrow the type if possible.If callers always pass
Prisma.Decimal, you can dropDecimalJsLikefrom the signature to simplify types.
459-462: Optional: de‑dupe repeated updates per tx.Multiple related addresses can trigger duplicate status changes for the same paymentId. Consider tracking a Set per processed tx.
Example:
- for (const addressWithTransaction of addressesWithTransactions) { + const seen = new Set<string>() + for (const addressWithTransaction of addressesWithTransactions) { + const parsed = parseOpReturnData(addressWithTransaction.transaction.opReturn ?? '') as any + const pid = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + if (pid === '' || seen.has(pid)) continue + seen.add(pid) // ... }Also applies to: 479-481
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prisma-local/migrations/20250912183436_client_payment/migration.sql(1 hunks)prisma-local/schema.prisma(2 hunks)services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(433-461)services/transactionService.ts (2)
getClientPayment(1011-1016)updatePaymentStatus(1004-1009)
🔇 Additional comments (3)
prisma-local/schema.prisma (2)
25-25: LGTM: added back‑relation on Address.
clientPayments ClientPayment[]correctly models the inverse relation.
273-277: LGTM: enum covers current lifecycle.
PENDING | ADDED_TO_MEMPOOL | CONFIRMEDmatches the flow described in the PR.services/chronikService.ts (1)
14-17: Imports look good.Using
updatePaymentStatus/getClientPaymenthere is appropriate for the new lifecycle.
services/chronikService.ts
Outdated
| private async updateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { | ||
| const parsedOpReturn = parseOpReturnData(opReturn ?? '') | ||
| const paymentId = parsedOpReturn.paymentId | ||
|
|
||
| const clientPayment = await getClientPayment(paymentId) | ||
| if (clientPayment.amount !== null) { | ||
| if (Number(clientPayment.amount) === Number(txAmount)) { | ||
| await updatePaymentStatus(paymentId, status) | ||
| } | ||
| } else { | ||
| await updatePaymentStatus(paymentId, status) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm expected paymentId format.
If generatePaymentId uses a different length/format, adjust the regex accordingly.
🏁 Script executed:
#!/bin/bash
# Inspect paymentId generation and OP_RETURN parsing to confirm format/length.
rg -nP "generatePaymentId|CreatePaymentId|paymentId" -C3
rg -nP "paymentId[^\n]*@id" prisma-local/schema.prismaLength of output: 27906
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== show generatePaymentId in services/transactionService.ts ==="
nl -ba services/transactionService.ts | sed -n '960,1005p' || true
echo
echo "=== show OP_RETURN parsing in services/chronikService.ts (full parse function) ==="
nl -ba services/chronikService.ts | sed -n '1,140p' || true
echo
echo "=== show updateClientPaymentStatus in services/chronikService.ts ==="
nl -ba services/chronikService.ts | sed -n '416,440p' || true
echo
echo "=== look for explicit regex/validation for paymentId in repo ==="
rg -n --hidden -S "\\b[0-9a-fA-F]{32}\\b|validate.*paymentId|is.*paymentId|paymentId.*match|paymentId.*test" || trueLength of output: 521
Confirm paymentId format and validate before DB lookup.
generatePaymentId() (services/transactionService.ts) produces a UUIDv4 with dashes removed (32 hex characters). parseOpReturn() (services/chronikService.ts) builds the paymentId from raw OP_RETURN bytes and can be variable length or ''. updateClientPaymentStatus (services/chronikService.ts — lines ~424–436) calls getClientPayment(paymentId) without validating/normalizing the parsed value, which can throw or mismatch.
- Enforce/normalize paymentId to /^[0-9a-f]{32}$/ before calling getClientPayment; if invalid or empty, skip/update no-op and log. Apply the check either in parseOpReturn (where ret.paymentId is set) or at the start of updateClientPaymentStatus.
- If OP_RETURN is intended to carry a shorter binary ID, decide and document the canonical format and update generatePaymentId / DB lookup accordingly.
🤖 Prompt for AI Agents
In services/chronikService.ts around lines 424 to 436, the parsed paymentId from
OP_RETURN can be empty or variable-length and is used directly in
getClientPayment; validate and normalize it before DB lookup by: coerce the
parsed paymentId to a lowercase hex string, strip any dashes, then verify it
matches /^[0-9a-f]{32}$/; if it fails validation (empty or wrong
length/characters) log a warning and return/do nothing; only call
getClientPayment/updatePaymentStatus when the normalized ID passes the regex.
Ensure the validation is added at the start of updateClientPaymentStatus (or
alternatively enforce/normalize in parseOpReturn) so DB lookups never receive
malformed IDs.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
services/triggerService.ts (2)
445-449: Type mismatch and potential JSON.stringify failure in logs.
PostDataTriggerLogdeclarespostedData/responseDataas string, but objects are assigned here, which will fail TS checks and may throw when stringified (e.g., BigInt). Stringify explicitly and defensively.- const responseData = await response.data + const responseData = response.data logData = { - postedData: parsedPostDataParameters, + postedData: JSON.stringify(parsedPostDataParameters), postedURL: trigger.postURL, - responseData + responseData: typeof responseData === 'string' ? responseData : JSON.stringify(responseData) }
436-443: Harden outbound POST against SSRF and oversized responses.User-controlled URLs are posted to directly. Add URL validation (https only, block private/link‑local IPs, limit redirects) and cap response sizes.
- const response = await axios.post( - trigger.postURL, + // Basic SSRF guard + const url = new URL(trigger.postURL) + if (url.protocol !== 'https:') { + throw new Error('POST URL must use https') + } + const response = await axios.post( + url.toString(), parsedPostDataParameters, { - timeout: config.triggerPOSTTimeout + timeout: config.triggerPOSTTimeout, + maxRedirects: 3, + maxContentLength: 1_000_000, // 1 MB + maxBodyLength: 1_000_000 } )Add a helper (outside this hunk) to DNS-resolve and block private ranges if you need stronger guarantees:
// helpers/net.ts export async function isPrivateAddress(hostname: string): Promise<boolean> { /* resolve + CIDR check */ }
🧹 Nitpick comments (12)
.github/coding-instructions.md (1)
12-12: Fix markdown list indentation (MD007).Adjust indentation to satisfy markdownlint (expected 0).
- - Make sure new tests added into the "integration-tests" directory are actually integration tests. +- Make sure new tests added into the "integration-tests" directory are actually integration tests.services/triggerService.ts (2)
406-435: Good pre-validation + early return; minor nits.
- Type the parsed payload as unknown/Record to avoid leaking any.
- Error log message “should never happen” is misleading; recommend actionable text with trigger id and error.message.
- let parsedPostDataParameters: any + let parsedPostDataParameters: Record<string, unknown> @@ - console.error(`[ERROR] Invalid trigger data in DB for trigger ${trigger.id} (should never happen)`) + console.error(`[TRIGGER] Invalid trigger postData for trigger ${trigger.id}: ${jsonErr?.message}`)
444-444: Remove unnecessary await on response.data.
response.datais not a promise.- const responseData = await response.data + const responseData = response.datatests/unittests/triggerService.test.ts (4)
56-110: “Execution scenarios” use console logs and never hit service paths.Drive the real code path and assert side effects; drop console logs.
- console.log('=== Test Case 1: Valid JSON ===') - ... - console.log('\n=== Test Case 2: Invalid JSON ===') + // Valid path -> one POST, non-error log + mockedParseTriggerPostData.mockReturnValue({ amount: 100, currency: 'XEC' }) + mockedAxios.post.mockResolvedValue({ data: { ok: true } }) + await __testOnly__.postDataForTrigger(trigger, {} as any) + expect(mockedAxios.post).toHaveBeenCalledTimes(1) + expect(prismaMock.triggerLog.create).toHaveBeenLastCalledWith(expect.objectContaining({ data: expect.objectContaining({ isError: false }) })) + + // Invalid path -> zero POSTs, error log + jest.clearAllMocks() + mockedParseTriggerPostData.mockImplementation(() => { throw new Error('boom') }) + await __testOnly__.postDataForTrigger(trigger, {} as any) + expect(mockedAxios.post).not.toHaveBeenCalled() + expect(prismaMock.triggerLog.create).toHaveBeenCalledWith(expect.objectContaining({ data: expect.objectContaining({ isError: true }) }))
111-153: Error-shape assertions don’t reflect real parser behavior.The real
parseTriggerPostDatawraps errors withRESPONSE_MESSAGES, so expecting nativeSyntaxErroris brittle. Prefer message substring/HTTP code mapping produced by the helper, or exercise service logging side-effects instead.
155-192: Edge-case tests still only exercise the mock.Same issue: call the service and assert no network call + error log persisted.
195-236: Avoid testing Axios directly; assert service behavior and counts.Count
mockedAxios.postcalls made by the service, not by the test. Remove console noise.- await mockedAxios.post('https://example.com', { amount: 100 }) - console.log('✅ Valid JSON: Network request made') + await __testOnly__.postDataForTrigger(trigger, {} as any) + expect(mockedAxios.post).toHaveBeenCalledTimes(1) @@ - console.log(`Network requests made: ${networkRequestCount}`) - expect(networkRequestCount).toBe(1) + expect(mockedAxios.post).toHaveBeenCalledTimes(1)utils/files.ts (5)
153-154: Avoid accumulating empty arrays in group map.Delete processed groups to prevent unbounded growth of
tempTxGroups.- this.tempTxGroups[groupKey] = [] + delete this.tempTxGroups[groupKey]
269-283: Stabilize group-key construction: sort names to ensure deterministic keys.Different iteration orders can produce different group keys for the same set of paybuttons.
- const uniqueButtonNames = new Set( - tx.address.paybuttons - .filter(pb => pb.paybutton.providerUserId === userId) - .map(pb => pb.paybutton.name) - ) - - if (uniqueButtonNames.size > 1) { + const uniqueButtonNames = new Set( + tx.address.paybuttons + .filter(pb => pb.paybutton.providerUserId === userId) + .map(pb => pb.paybutton.name) + ) + const names = Array.from(uniqueButtonNames).sort() + + if (names.length > 1) { if (paybuttonId !== undefined) { return tx.address.paybuttons.find(pb => pb.paybutton.id === paybuttonId)?.paybutton.name ?? '' } else { - return [...uniqueButtonNames].join('_') + return names.join('_') } } else { - return uniqueButtonNames.values().next().value ?? '' + return names[0] ?? '' }Note: Using
_as a delimiter remains fragile if names include_. Consider encoding names (e.g., encodeURIComponent) or using a structured/base64 key in a follow-up.
147-152: Deduplicate single-tx push logic to one helper.
TransactionGroupManager.addSingleTransactionduplicatesaddSingleTransactionToResults, risking drift.- if (tempTxGroup.length === 1) { - this.addSingleTransaction(tempTxGroup[0], groupKey, treatedPayments) - } else { + if (tempTxGroup.length === 1) { + addSingleTransactionToResults(tempTxGroup[0], groupKey, this.currency, treatedPayments, this.timezone) + } else { this.addCollapsedTransactionGroup(tempTxGroup, groupKey, treatedPayments) }- private addSingleTransaction( - tx: TransactionsWithPaybuttonsAndPrices, - groupKey: string, - treatedPayments: TransactionFileData[] - ): void { - const { timestamp, hash, address, amount } = tx - const values = getTransactionValue(tx) - const value = Number(values[this.currency]) - const rate = tx.prices.find(p => p.price.quoteId === QUOTE_IDS[this.currency.toUpperCase()])!.price.value - const buttonNames = this.extractButtonNamesFromGroupKey(groupKey) - - treatedPayments.push({ - amount, - value, - date: moment.tz(timestamp * 1000, this.timezone), - transactionId: hash, - rate, - currency: this.currency, - address: address.address, - notes: buttonNames, - newtworkId: address.networkId - } as TransactionFileData) - } + // Use top-level addSingleTransactionToResults to keep a single source of truth.Also applies to: 156-178
324-324: Remove unused variable.
timestampis never used.- const { timestamp } = tx
205-208: Group-key parsing viasplit('_')is brittle.If button names contain
_, parsing yields incorrect notes. Prefer structured encoding (e.g., base64(JSON)) or at least encode names.If you opt for minimal change now, switch to
encodeURIComponentfor names ingenerateGroupKeyanddecodeURIComponentin the extractors, or adopt a structured key in a follow-up.Also applies to: 297-298
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/coding-instructions.md(1 hunks)package.json(1 hunks)services/triggerService.ts(1 hunks)tests/unittests/triggerService.test.ts(1 hunks)utils/files.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (3)
services/triggerService.ts (1)
utils/validators.ts (1)
parseTriggerPostData(261-287)
tests/unittests/triggerService.test.ts (2)
utils/validators.ts (1)
parseTriggerPostData(261-287)prisma-local/mockedClient.ts (1)
prismaMock(19-19)
utils/files.ts (2)
services/transactionService.ts (2)
TransactionsWithPaybuttonsAndPrices(134-134)getTransactionValue(15-32)constants/index.ts (4)
SupportedQuotesType(163-163)QUOTE_IDS(205-205)DEFAULT_MULTI_VALUES_LINE_LABEL(266-266)RESPONSE_MESSAGES(8-107)
🪛 ESLint
utils/files.ts
[error] 125-125: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 130-130: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 137-137: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 143-143: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 156-156: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 180-180: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 205-205: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 209-209: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 212-212: Trailing spaces not allowed.
(no-trailing-spaces)
[error] 225-225: Missing space before function parentheses.
(@typescript-eslint/space-before-function-paren)
[error] 324-324: 'timestamp' is assigned a value but never used.
(@typescript-eslint/no-unused-vars)
🪛 markdownlint-cli2 (0.17.2)
.github/coding-instructions.md
12-12: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🔇 Additional comments (2)
utils/files.ts (2)
336-338: Flushing all groups on a big tx may split unrelated groups; flush only the current group.This can prematurely flush other active groups and fragment later collapses.
- groupManager.processAllGroups(treatedPayments) + groupManager.processGroup(groupKey, treatedPayments) addSingleTransactionToResults(tx, groupKey, currency, treatedPayments, timezone)Please confirm expected ordering vs prior behavior; if ordering across groups must be preserved, we may need an ordered queue rather than global flushes.
125-125: Lint: fix spacing before parens and trailing whitespace.ESLint flags space-before-function-paren and trailing-space issues in utils/files.ts — fix at lines: 125, 130, 137, 143, 156, 180, 205, 209, 212, 225. Auto-fix couldn't be run here (pnpm/npm/yarn reported no "lint" script); run your linter locally (e.g. npx eslint --fix utils/files.ts or add a lint script) and re-run checks.
| describe('Trigger JSON Validation Unit Tests', () => { | ||
| describe('Trigger Creation with JSON Validation', () => { | ||
| it('should reject trigger creation with invalid JSON during validation', async () => { | ||
| const invalidPostData = '{"amount": <amount>, "currency": <currency>' | ||
|
|
||
| mockedParseTriggerPostData.mockImplementation(() => { | ||
| throw new SyntaxError('Unexpected end of JSON input') | ||
| }) | ||
|
|
||
| expect(() => { | ||
| parseTriggerPostData({ | ||
| userId: 'test-user', | ||
| postData: invalidPostData, | ||
| postDataParameters: {} as any | ||
| }) | ||
| }).toThrow('Unexpected end of JSON input') | ||
|
|
||
| expect(mockedParseTriggerPostData).toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('should allow trigger creation with valid JSON', async () => { | ||
| const validPostData = '{"amount": <amount>, "currency": <currency>}' | ||
| const expectedParsedData = { amount: 100, currency: 'XEC' } | ||
|
|
||
| mockedParseTriggerPostData.mockReturnValue(expectedParsedData) | ||
|
|
||
| const result = parseTriggerPostData({ | ||
| userId: 'test-user', | ||
| postData: validPostData, | ||
| postDataParameters: {} as any | ||
| }) | ||
|
|
||
| expect(result).toEqual(expectedParsedData) | ||
| expect(mockedParseTriggerPostData).toHaveBeenCalled() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
These tests mock and then “test” the mock, not the service.
You’re asserting behavior of your own mocked parseTriggerPostData; no service code is exercised. Rework to call the function that uses parsing (e.g., postDataForTrigger) and assert DB log + network side effects.
Recommended approach:
- Re-export
postDataForTriggerfor tests (e.g.,export const __testOnly__ = { postDataForTrigger }), or inject it. - Assert
prisma.triggerLog.createandaxios.postcalls per path.
Example (after re-export):
- expect(() => { parseTriggerPostData(...)}).toThrow(...)
+ const trigger = { id:'t1', isEmailTrigger:false, postURL:'https://example.com', postData:'{}', paybutton:{ name:'btn', providerUserId:'u1' } } as any
+ mockedParseTriggerPostData.mockImplementation(() => { throw new Error('Invalid JSON') })
+ await expect(__testOnly__.postDataForTrigger(trigger, {} as any)).resolves.toBeUndefined()
+ expect(prismaMock.triggerLog.create).toHaveBeenCalledWith(expect.objectContaining({ data: expect.objectContaining({ isError: true }) }))
+ expect(mockedAxios.post).not.toHaveBeenCalled()Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/unittests/triggerService.test.ts around lines 19 to 54, the current
tests only exercise the mocked parseTriggerPostData and assert the mock rather
than the service behavior; change them to call the actual service function that
uses parsing (postDataForTrigger) so side effects are tested. Re-export or
otherwise expose postDataForTrigger (e.g., export __testOnly__ = {
postDataForTrigger }) or inject the parser into the service, then in the tests
mock parseTriggerPostData only as an input dependency, call postDataForTrigger
with the same payloads, and assert the expected side effects: that
prisma.triggerLog.create was invoked with the correct args and that axios.post
was called for the success path (or not called on parse error). Remove
assertions that directly test mockedParseTriggerPostData and instead assert
service-level outcomes (DB log + network calls and thrown errors).
| newtworkId: address.networkId | ||
| } as TransactionFileData) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Fix typo: newtworkId → networkId (propagate consistently).
The misspelling leaks into output and helper usage; standardize to networkId.
Apply in these spots:
- newtworkId: address.networkId
+ networkId: address.networkId- newtworkId: tempTxGroup[0].address.networkId,
+ networkId: tempTxGroup[0].address.networkId,- newtworkId: address.networkId
+ networkId: address.networkIdThen update:
TransactionFileDatainterface (newtworkId→networkId)formatPaybuttonTransactionsFileData(readnetworkId)- Any other references.
Run to locate remaining occurrences:
Also applies to: 200-201, 309-309
🏁 Script executed:
#!/bin/bash
rg -n --color=never '\bnewtworkId\b' -S -C2Length of output: 1304
Fix typo: rename newtworkId → networkId and propagate across utils/files.ts
Rename the interface property and all usages (TransactionFileData, formatPaybuttonTransactionsFileData, callers). Replace occurrences like:
- newtworkId: address.networkId
+ networkId: address.networkId- newtworkId: tempTxGroup[0].address.networkId,
+ networkId: tempTxGroup[0].address.networkId,- newtworkId: address.networkId
+ networkId: address.networkIdOccurrences to update (from grep): utils/files.ts lines 30, 62, 64, 176, 200, 308, 399. Also update any exported types/serializers that read/write this field and adjust callers/tests that rely on the old name.
📝 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.
| newtworkId: address.networkId | |
| } as TransactionFileData) | |
| networkId: address.networkId | |
| } as TransactionFileData) |
🤖 Prompt for AI Agents
In utils/files.ts around lines 176-177 (and also update lines 30, 62, 64, 200,
308, 399), the TransactionFileData property is misspelled as "newtworkId" —
rename it to "networkId" across the file: update the interface/type declaration,
all references in formatPaybuttonTransactionsFileData and callers, plus any
serializers/deserializers or exported types that read/write this field; run a
project-wide replace for "newtworkId"→"networkId", update affected tests/call
sites to use the new name, and ensure any JSON (de)serialization keys are
adjusted accordingly so runtime reads/writes and typings stay consistent.
de45d6b to
cd36580
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
services/chronikService.ts (3)
484-487: Call the correct method and pass the address for cross-address safety.
this.updateClientPaymentStatuswas not defined (method was named differently) and lacked the address guard. Update the call to use the class method with address.- const { amount, opReturn } = addressWithTransaction.transaction - await this.updateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus) + const { amount, opReturn } = addressWithTransaction.transaction + await this.updateClientPaymentStatus( + amount as Prisma.Decimal, + opReturn, + 'CONFIRMED' as ClientPaymentStatus, + addressWithTransaction.address.address + )
504-506: Same fix for mempool path + rely on callee to prevent downgrades.Pass the address and let the updater guard against downgrades.
- const { amount, opReturn } = addressWithTransaction.transaction - const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus - await this.updateClientPaymentStatus(amount, opReturn, newClientPaymentStatus) + const { amount, opReturn } = addressWithTransaction.transaction + await this.updateClientPaymentStatus( + amount as Prisma.Decimal, + opReturn, + 'ADDED_TO_MEMPOOL' as ClientPaymentStatus, + addressWithTransaction.address.address + )
449-461: Harden payment-status updater: fix decimals, validation, downgrades, and address check.
- Use Prisma.Decimal.equals, not Number coercion.
- Validate
paymentIdbefore DB calls; ignore unknown IDs.- Prevent downgrades (CONFIRMED → ADDED_TO_MEMPOOL).
- Ensure tx address matches the ClientPayment’s address.
- Rename method to match call sites and use aliased DB updater.
Apply:
- private async handleUpdateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { - const parsedOpReturn = parseOpReturnData(opReturn ?? '') - const paymentId = parsedOpReturn.paymentId - - const clientPayment = await getClientPayment(paymentId) - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - await updateClientPaymentStatus(paymentId, status) - } - } + private async updateClientPaymentStatus( + txAmount: string | number | Prisma.Decimal | DecimalJsLike, + opReturn: string | undefined, + status: ClientPaymentStatus, + address?: string + ): Promise<void> { + // opReturn is stored as JSON from getNullDataScriptData() + let parsed: any + try { + parsed = JSON.parse(opReturn ?? '') + } catch { + return + } + const paymentId: string = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + if (!/^[0-9a-fA-F]{32}$/.test(paymentId)) return + + let clientPayment + try { + clientPayment = await getClientPayment(paymentId) + } catch { + return // unknown paymentId + } + if (address && clientPayment.address.address !== address) return + if (clientPayment.status === 'CONFIRMED' && status !== 'CONFIRMED') return + if (clientPayment.status === status) return + + if (clientPayment.amount !== null) { + const txDec = new Prisma.Decimal(txAmount as any) + if (!clientPayment.amount.equals(txDec)) return + } + await updateClientPaymentStatusDb(paymentId, status) + }
🧹 Nitpick comments (2)
services/chronikService.ts (2)
482-487: Optional: de-dupe repeated paymentId updates per tx.If the same paymentId appears on multiple related addresses for one tx, you can avoid redundant DB calls with a Set.
- for (const addressWithTransaction of addressesWithTransactions) { + const seen = new Set<string>() + for (const addressWithTransaction of addressesWithTransactions) { const { amount, opReturn } = addressWithTransaction.transaction - await this.updateClientPaymentStatus( + const parsed = (() => { try { return JSON.parse(opReturn ?? '') } catch { return {} } })() + const pid = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + if (pid && !seen.has(pid)) seen.add(pid); else continue + await this.updateClientPaymentStatus( amount as Prisma.Decimal, opReturn, 'CONFIRMED' as ClientPaymentStatus, addressWithTransaction.address.address ) }
449-461: Minor: prefer direct JSON.parse over parseOpReturnData for opReturn.
opReturnhere is the JSON emitted bygetNullDataScriptData; usingJSON.parseavoids the lenient KV/array fallbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
pages/api/payments/paymentId/index.ts(1 hunks)prisma-local/migrations/20250912183436_client_payment/migration.sql(1 hunks)prisma-local/migrations/migration_lock.toml(1 hunks)prisma-local/schema.prisma(2 hunks)services/chronikService.ts(5 hunks)services/transactionService.ts(3 hunks)utils/validators.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- prisma-local/migrations/20250912183436_client_payment/migration.sql
- utils/validators.ts
- prisma-local/schema.prisma
- prisma-local/migrations/migration_lock.toml
- services/transactionService.ts
- pages/api/payments/paymentId/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (1)
getClientPayment(1012-1017)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
services/chronikService.ts (2)
18-18: LGTM — needed types are imported.
33-33: LGTM —DecimalJsLikeimport is appropriate.
lissavxo
force-pushed
the
feat/update-payment-id
branch
from
cd36580 to
d0d25cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
services/chronikService.ts (1)
14-17: Alias DB updater to avoid confusion.Alias the imported DB function to make call sites explicit and avoid confusion with class helpers.
connectAllTransactionsToPrices, - updateClientPaymentStatus, + updateClientPaymentStatus as updateClientPaymentStatusDb, getClientPayment
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (1)
getClientPayment(1012-1017)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (6)
services/chronikService.ts (6)
18-18: LGTM on Prisma/Status imports.
33-34: LGTM on DecimalJsLike import (used in helper signature).
467-471: Good: precomputes input/output addresses once per tx (fixes prior undefined).This addresses the previous “undefined outputAddresses” issue and reduces repeated work.
449-461: Harden payment status updates: validate paymentId, compare Decimals correctly, guard downgrades, verify address, and handle unknown IDs.
- Validate paymentId (32 hex) before DB access.
- Wrap getClientPayment in try/catch; ignore unknown IDs instead of throwing.
- Use Prisma.Decimal.equals() for amount comparison (avoid Number() coercion).
- Prevent downgrades (skip if current is CONFIRMED and new is mempool).
- Skip redundant updates (same status).
- Ensure tx address matches ClientPayment.address to prevent cross‑address updates.
- Call the aliased DB updater.
- private async handleUpdateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { - const parsedOpReturn = parseOpReturnData(opReturn ?? '') - const paymentId = parsedOpReturn.paymentId - - const clientPayment = await getClientPayment(paymentId) - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - await updateClientPaymentStatus(paymentId, status) - } - } + private async handleUpdateClientPaymentStatus ( + txAmount: string | number | Prisma.Decimal | DecimalJsLike, + opReturn: string | undefined, + status: ClientPaymentStatus, + txAddress: string + ): Promise<void> { + const parsed = parseOpReturnData(opReturn ?? '') as any + const rawPaymentId = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + const paymentId = rawPaymentId.replace(/-/g, '').toLowerCase() + if (!/^[0-9a-f]{32}$/.test(paymentId)) return + try { + const clientPayment = await getClientPayment(paymentId) + // Guard wrong address, downgrades, and redundant updates + if (clientPayment.address.address !== txAddress) return + if (clientPayment.status === 'CONFIRMED' && status !== 'CONFIRMED') return + if (clientPayment.status === status) return + if (clientPayment.amount !== null) { + const txDec = txAmount instanceof Prisma.Decimal ? txAmount : new Prisma.Decimal(txAmount as any) + if (clientPayment.amount.equals(txDec)) { + await updateClientPaymentStatusDb(paymentId, status) + } + } else { + await updateClientPaymentStatusDb(paymentId, status) + } + } catch { + // Unknown paymentId — ignore + return + } + }
486-489: Pass tx address into helper; optional: dedupe by paymentId per tx.
- Provide the tx address so the helper can verify it matches the ClientPayment address.
- (Optional) Consider deduping by paymentId within this block to avoid duplicate updates across multiple related addresses.
- for (const addressWithTransaction of addressesWithTransactions) { - const { amount, opReturn } = addressWithTransaction.transaction - await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus) - } + for (const addressWithTransaction of addressesWithTransactions) { + const { amount, opReturn } = addressWithTransaction.transaction + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + 'CONFIRMED' as ClientPaymentStatus, + addressWithTransaction.address.address + ) + }
506-509: Same fix for mempool: pass tx address; downgrades are guarded in helper.- const { amount, opReturn } = addressWithTransaction.transaction - const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus - await this.handleUpdateClientPaymentStatus(amount, opReturn, newClientPaymentStatus) + const { amount, opReturn } = addressWithTransaction.transaction + const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + newClientPaymentStatus, + addressWithTransaction.address.address + )
lissavxo
force-pushed
the
feat/update-payment-id
branch
from
5f8fe19 to
23bf29f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/chronikService.ts(5 hunks)services/transactionService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1012-1017)updateClientPaymentStatus(1005-1010)
services/transactionService.ts (3)
constants/index.ts (1)
NETWORK_IDS_FROM_SLUGS(131-134)services/addressService.ts (1)
addressExists(127-141)services/chronikService.ts (1)
multiBlockchainClient(914-914)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (10)
services/transactionService.ts (4)
2-3: Imports look fine.Types/constants import aligns with new usage.
1005-1010: Updater is OK as a thin DAO.Idempotency and downgrade prevention are handled by callers; this is fine.
1012-1017: Getter looks good.Appropriate include of address for downstream checks.
970-1003: Normalize/validate address and guard unknown network in generatePaymentId; use canonical address; avoid cycle via lazy import.Prevents subtle bugs and bad DB writes; also avoids cross‑module init hazards.
Apply:
export const generatePaymentId = async (address: string, amount?: Prisma.Decimal): Promise<string> => { const rawUUID = uuidv4() const cleanUUID = rawUUID.replace(/-/g, '') const status = 'PENDING' as ClientPaymentStatus - const prefix = address.split(':')[0].toLowerCase() - const networkId = NETWORK_IDS_FROM_SLUGS[prefix] - const isAddressRegistered = await addressExists(address) + const normalizedAddress = parseAddress(address) + const prefix = normalizedAddress.split(':')[0].toLowerCase() + const networkId = NETWORK_IDS_FROM_SLUGS[prefix] + if (networkId === undefined) { + throw new Error(RESPONSE_MESSAGES.INVALID_NETWORK_SLUG_400.message) + } + const isAddressRegistered = await addressExists(normalizedAddress) const clientPayment = await prisma.clientPayment.create({ data: { address: { connectOrCreate: { - where: { address }, + where: { address: normalizedAddress }, create: { - address, + address: normalizedAddress, networkId } } }, paymentId: cleanUUID, status, amount }, include: { address: true } }) - if (!isAddressRegistered) { - void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) - } + if (!isAddressRegistered) { + // Lazy import to avoid circular dependency with chronikService + void import('services/chronikService') + .then(({ multiBlockchainClient }) => multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address])) + .catch(() => {}) + } return clientPayment.paymentId }services/chronikService.ts (6)
14-17: Import usage is coherent now.Using service-level update/get avoids earlier name collision issues.
33-34: OK to import DecimalJsLike.Type matches the helper signature.
467-471: Good: precomputes inputs/outputs once per tx.Fixes previous undefined outputAddresses issue and reduces duplicate work.
506-509: Same fix for mempool path; pass tx address.Also relies on handle() to avoid downgrades/duplicates.
Apply:
- const { amount, opReturn } = addressWithTransaction.transaction - const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus - await this.handleUpdateClientPaymentStatus(amount, opReturn, newClientPaymentStatus) + const { amount, opReturn } = addressWithTransaction.transaction + const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + newClientPaymentStatus, + addressWithTransaction.address.address + )
486-489: Pass tx address into validator.Needed to prevent cross‑address updates per the new handle signature.
Apply:
- const { amount, opReturn } = addressWithTransaction.transaction - await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus) + const { amount, opReturn } = addressWithTransaction.transaction + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + 'CONFIRMED' as ClientPaymentStatus, + addressWithTransaction.address.address + )
449-461: Harden client payment updates: validate paymentId, compare Decimals correctly, ensure address matches, and prevent downgrades.Current code can throw on missing/invalid paymentId, mis-update across addresses, and compares Decimals via Number(). Add guards and proper Decimal equality; also avoid downgrading CONFIRMED to ADDED_TO_MEMPOOL and skip no-ops.
Apply:
- private async handleUpdateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { - const parsedOpReturn = parseOpReturnData(opReturn ?? '') - const paymentId = parsedOpReturn.paymentId - - const clientPayment = await getClientPayment(paymentId) - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - await updateClientPaymentStatus(paymentId, status) - } - } + private async handleUpdateClientPaymentStatus ( + txAmount: string | number | Prisma.Decimal | DecimalJsLike, + opReturn: string | undefined, + status: ClientPaymentStatus, + txAddress: string + ): Promise<void> { + const parsed = parseOpReturnData(opReturn ?? '') + const paymentIdRaw = typeof parsed?.paymentId === 'string' ? parsed.paymentId : '' + const paymentId = paymentIdRaw.replace(/-/g, '').toLowerCase() + if (!/^[0-9a-f]{32}$/.test(paymentId)) return + try { + const clientPayment = await getClientPayment(paymentId) + // Avoid cross-address updates and downgrades/no-ops + if (clientPayment.address.address !== txAddress) return + if (clientPayment.status === status) return + if (clientPayment.status === 'CONFIRMED' && status !== 'CONFIRMED') return + if (clientPayment.amount !== null) { + const txDec = new Prisma.Decimal(txAmount as any) + if (!clientPayment.amount.equals(txDec)) return + } + await updateClientPaymentStatus(paymentId, status) + } catch { + // Unknown/malformed paymentId — ignore + } + }
| import { v4 as uuidv4 } from 'uuid' | ||
| import { multiBlockchainClient } from 'services/chronikService' | ||
|
|
There was a problem hiding this comment.
Break circular dependency with chronikService.
Top-level import of multiBlockchainClient creates a cycle (chronikService → transactionService → chronikService) that can yield undefined bindings at runtime. Lazy import inside the function avoids it.
Apply:
-import { v4 as uuidv4 } from 'uuid'
-import { multiBlockchainClient } from 'services/chronikService'
+import { v4 as uuidv4 } from 'uuid'Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In services/transactionService.ts around lines 12 to 14, the top-level import of
multiBlockchainClient creates a circular dependency with chronikService; remove
the top-level import and perform a lazy/dynamic import of multiBlockchainClient
inside the function(s) that use it (e.g., call const { multiBlockchainClient } =
await import('services/chronikService') or use require(...) if not in an async
context) so the module is resolved at call-time and avoids undefined bindings at
runtime; update all call sites in this file to use the locally-imported
multiBlockchainClient instance.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
services/transactionService.ts (1)
976-976: Consider using a database transaction for atomicityThe function checks
addressExistsoutside the Prisma create operation, which could lead to race conditions if the address is created between the check and the create operation.Consider wrapping the entire operation in a transaction to ensure atomicity:
export const generatePaymentId = async (address: string, amount?: Prisma.Decimal): Promise<string> => { const rawUUID = uuidv4() const cleanUUID = rawUUID.replace(/-/g, '') const normalizedAddress = parseAddress(address) const prefix = normalizedAddress.split(':')[0].toLowerCase() const networkId = NETWORK_IDS_FROM_SLUGS[prefix] if (networkId === undefined) { throw new Error(RESPONSE_MESSAGES.INVALID_NETWORK_SLUG_400.message) } return await prisma.$transaction(async (tx) => { const isAddressRegistered = await tx.address.findUnique({ where: { address: normalizedAddress } }) !== null const clientPayment = await tx.clientPayment.create({ data: { address: { connectOrCreate: { where: { address: normalizedAddress }, create: { address: normalizedAddress, networkId } } }, paymentId: cleanUUID, status: 'PENDING', amount }, include: { address: true } }) if (!isAddressRegistered) { const { multiBlockchainClient } = await import('services/chronikService') void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) } return clientPayment.paymentId }) }Also applies to: 991-991
services/chronikService.ts (3)
18-18: Consider importing Prisma module more preciselyInstead of importing the entire Prisma namespace, consider importing only what you need.
-import { Address, Prisma, ClientPaymentStatus } from '@prisma/client' +import { Address, ClientPaymentStatus } from '@prisma/client' +import type { Prisma } from '@prisma/client'
33-33: Remove unused importThe
DecimalJsLiketype appears to be imported but not actually used in a meaningful way in the code.-import { DecimalJsLike } from '@prisma/client/runtime/library'
449-464: Add logging for better observabilityConsider adding structured logging to track client payment status updates for debugging and monitoring.
private async handleUpdateClientPaymentStatus (...) { // ... existing code ... console.log(`[CLIENT_PAYMENT] Updating payment ${paymentId} status from ${clientPayment.status} to ${status}`) await updateClientPaymentStatus(paymentId, status) console.log(`[CLIENT_PAYMENT] Successfully updated payment ${paymentId} to ${status}`) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
services/chronikService.ts(5 hunks)services/transactionService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/transactionService.ts (3)
constants/index.ts (1)
NETWORK_IDS_FROM_SLUGS(131-134)services/addressService.ts (1)
addressExists(127-141)services/chronikService.ts (1)
multiBlockchainClient(917-917)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1012-1017)updateClientPaymentStatus(1005-1010)
🔇 Additional comments (4)
services/chronikService.ts (2)
470-474: Good improvement: Computing addresses earlyComputing
addressesWithTransactionsand helper addresses before the conditional branches ensures they're available for all message types.
510-512: Apply the same address validation for mempool transactionsThe mempool path should also validate addresses before updating payment status.
Update the mempool call to include address validation:
const { amount, opReturn } = addressWithTransaction.transaction const newClientPaymentStatus = 'ADDED_TO_MEMPOOL' as ClientPaymentStatus -await this.handleUpdateClientPaymentStatus(amount, opReturn, newClientPaymentStatus) +await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + newClientPaymentStatus, + addressWithTransaction.address.address +)services/transactionService.ts (2)
970-1003: Validate and normalize address; guard unknown network; avoid subtle bugs ingeneratePaymentIdThe function should validate and normalize the input address before DB operations to prevent issues with different address formats and invalid networks.
Apply:
export const generatePaymentId = async (address: string, amount?: Prisma.Decimal): Promise<string> => { const rawUUID = uuidv4() const cleanUUID = rawUUID.replace(/-/g, '') const status = 'PENDING' as ClientPaymentStatus - const prefix = address.split(':')[0].toLowerCase() - const networkId = NETWORK_IDS_FROM_SLUGS[prefix] - const isAddressRegistered = await addressExists(address) + const normalizedAddress = parseAddress(address) + const prefix = normalizedAddress.split(':')[0].toLowerCase() + const networkId = NETWORK_IDS_FROM_SLUGS[prefix] + if (networkId === undefined) { + throw new Error(RESPONSE_MESSAGES.INVALID_NETWORK_SLUG_400.message) + } + const isAddressRegistered = await addressExists(normalizedAddress) const clientPayment = await prisma.clientPayment.create({ data: { address: { connectOrCreate: { - where: { address }, + where: { address: normalizedAddress }, create: { - address, + address: normalizedAddress, networkId } } }, paymentId: cleanUUID, status, amount }, include: { address: true } }) if (!isAddressRegistered) { void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) } return clientPayment.paymentId }
12-13: Break circular dependency with chronikServiceThe top-level import of
multiBlockchainClientcreates a circular dependency (chronikService → transactionService → chronikService) that can yield undefined bindings at runtime.Consider lazy importing
multiBlockchainClientinside the function that uses it:import { v4 as uuidv4 } from 'uuid' -import { multiBlockchainClient } from 'services/chronikService'Then in
generatePaymentId:if (!isAddressRegistered) { + const { multiBlockchainClient } = await import('services/chronikService') void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) }
|
|
||
| export const updateClientPaymentStatus = async (paymentId: string, status: ClientPaymentStatus): Promise<void> => { | ||
| await prisma.clientPayment.update({ | ||
| where: { paymentId }, | ||
| data: { status } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add error handling for non-existent payment IDs
The updateClientPaymentStatus function should handle cases where the payment ID doesn't exist.
export const updateClientPaymentStatus = async (paymentId: string, status: ClientPaymentStatus): Promise<void> => {
- await prisma.clientPayment.update({
- where: { paymentId },
- data: { status }
- })
+ try {
+ await prisma.clientPayment.update({
+ where: { paymentId },
+ data: { status }
+ })
+ } catch (error: any) {
+ if (error.code === 'P2025') {
+ console.warn(`Client payment not found: ${paymentId}`)
+ } else {
+ throw error
+ }
+ }
}📝 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.
| export const updateClientPaymentStatus = async (paymentId: string, status: ClientPaymentStatus): Promise<void> => { | |
| await prisma.clientPayment.update({ | |
| where: { paymentId }, | |
| data: { status } | |
| }) | |
| } | |
| export const updateClientPaymentStatus = async (paymentId: string, status: ClientPaymentStatus): Promise<void> => { | |
| try { | |
| await prisma.clientPayment.update({ | |
| where: { paymentId }, | |
| data: { status } | |
| }) | |
| } catch (error: any) { | |
| if (error.code === 'P2025') { | |
| console.warn(`Client payment not found: ${paymentId}`) | |
| } else { | |
| throw error | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In services/transactionService.ts around lines 1004–1010, the
updateClientPaymentStatus currently calls prisma.clientPayment.update without
handling the case where paymentId doesn't exist; wrap the update in a try/catch
(or use prisma.clientPayment.updateMany and check the returned count) and handle
the "not found" case explicitly — on Prisma's P2025 error (or when count === 0)
either throw a clear NotFoundError or return a controlled result, and ensure the
function signature and callers are adjusted to expect/propagate that error.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/api/payments/paymentId/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/api/payments/paymentId/index.ts (4)
utils/index.ts (1)
runMiddleware(137-151)utils/validators.ts (2)
parseCreatePaymentIdPOSTRequest(578-590)parseAddress(31-48)services/transactionService.ts (1)
generatePaymentId(970-1003)constants/index.ts (1)
RESPONSE_MESSAGES(8-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (1)
pages/api/payments/paymentId/index.ts (1)
1-21: Construct a Prisma.Decimal before invokinggeneratePaymentId.Casting
values.amounttoDecimalleaves it as a string at runtime, so we're still passing a string intogeneratePaymentId, which expects aPrisma.Decimal. That breaks the function contract and can surface subtle precision/validation issues once the service uses decimal-specific operations. This was already called out in the prior review and still needs to be addressed. ImportPrismafrom@prisma/clientand build a realPrisma.Decimalinstead of type-casting.-import { Decimal } from '@prisma/client/runtime/library' +import { Prisma } from '@prisma/client' ... - const amount = values.amount as Decimal | undefined + const amount = values.amount !== undefined ? new Prisma.Decimal(values.amount) : undefined
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pages/api/payments/paymentId/index.ts (3)
13-13: Type the API handler with NextApiRequest/NextApiResponse.Improves type safety and DX.
-export default async (req: any, res: any): Promise<void> => { +export default async function handler(req: NextApiRequest, res: NextApiResponse): Promise<void> {Add this import near the top of the file:
import type { NextApiRequest, NextApiResponse } from 'next'
14-15: Optionally short‑circuit OPTIONS preflight.In some Next setups, this avoids falling through when CORS doesn’t end the response.
await runMiddleware(req, res, cors) + if (req.method === 'OPTIONS') { + res.status(200).end() + return + }
9-11: Optional CORS tweak: optionsSuccessStatus for legacy agents.Preflight 204 can trip some clients; 200 is safer.
const cors = Cors({ - methods: ['POST'] + methods: ['POST'], + optionsSuccessStatus: 200 })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/api/payments/paymentId/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/api/payments/paymentId/index.ts (4)
utils/index.ts (1)
runMiddleware(137-151)utils/validators.ts (2)
parseCreatePaymentIdPOSTRequest(578-590)parseAddress(31-48)services/transactionService.ts (1)
generatePaymentId(970-1003)constants/index.ts (1)
RESPONSE_MESSAGES(8-107)
🔇 Additional comments (2)
pages/api/payments/paymentId/index.ts (2)
1-1: Construct Prisma.Decimal instead of casting; import from '@prisma/client'.Casting keeps a string at runtime. Build a Prisma.Decimal and import Prisma from the public entrypoint.
-import { Decimal } from '@prisma/client/runtime/library' +import { Prisma } from '@prisma/client' @@ - const amount = values.amount as Decimal | undefined + const amount = values.amount !== undefined ? new Prisma.Decimal(values.amount) : undefinedAlso applies to: 19-19
25-31: Return 400 for invalid addresses instead of 500.Map INVALID_ADDRESS_400 explicitly to avoid misreporting as 500.
switch (error.message) { case RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400.message: res.status(RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400.statusCode).json(RESPONSE_MESSAGES.ADDRESS_NOT_PROVIDED_400) break + case RESPONSE_MESSAGES.INVALID_ADDRESS_400.message: + res.status(RESPONSE_MESSAGES.INVALID_ADDRESS_400.statusCode).json(RESPONSE_MESSAGES.INVALID_ADDRESS_400) + break default: res.status(500).json({ statusCode: 500, message: error.message })
| res.status(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED.statusCode) | ||
| .json(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED) | ||
| } |
There was a problem hiding this comment.
Respond 405 (not 500) and include the Allow header for unsupported methods.
Current constant yields 500. Use a proper 405.
- res.status(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED.statusCode)
- .json(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED)
+ res.setHeader('Allow', ['POST'])
+ res.status(405).json({ statusCode: 405, message: 'Method not allowed.' })📝 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.
| res.status(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED.statusCode) | |
| .json(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED) | |
| } | |
| res.setHeader('Allow', ['POST']) | |
| res.status(405).json({ statusCode: 405, message: 'Method not allowed.' }) | |
| } |
🤖 Prompt for AI Agents
In pages/api/payments/paymentId/index.ts around lines 34 to 36, the handler
currently returns the wrong constant that yields a 500 for unsupported HTTP
methods; change it to respond with HTTP 405 (Method Not Allowed) and set the
Allow header to the list of supported methods (e.g., "GET, POST" or whichever
methods this endpoint accepts). Replace the current
RESPONSE_MESSAGES.METHOD_NOT_ALLOWED constant with a proper 405 response (or
construct res.status(405).json(...)) and add res.setHeader('Allow',
'<allowed-methods>') before sending the response so clients receive both the 405
status and the Allow header.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
services/chronikService.ts (2)
595-598: Update call site to pass transaction address.The
handleUpdateClientPaymentStatuscall is missing the transaction address parameter needed for address verification (see method signature fix above).Apply this diff:
for (const addressWithTransaction of addressesWithTransactions) { const { amount, opReturn } = addressWithTransaction.transaction - await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus) + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + 'CONFIRMED' as ClientPaymentStatus, + addressWithTransaction.address.address + ) }
557-572: Critical: Multiple safety and correctness issues in payment status update logic.The method has several problems that can cause incorrect behavior:
- Missing paymentId validation: No format check for the 32-character hex UUID (line 559-560).
- Incorrect Decimal comparison: Using
Number()comparison (line 564) will fail for Prisma Decimal objects and may lose precision.- No address verification: The method doesn't verify that the transaction address matches the ClientPayment's registered address.
- Missing status downgrade prevention: No check to prevent downgrading from CONFIRMED to ADDED_TO_MEMPOOL.
- No error handling: Database operations can throw but aren't wrapped in try/catch.
Apply this diff to fix all issues:
-private async handleUpdateClientPaymentStatus (txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus): Promise<void> { +private async handleUpdateClientPaymentStatus ( + txAmount: string | number | Prisma.Decimal | DecimalJsLike, + opReturn: string | undefined, + status: ClientPaymentStatus, + txAddress: string +): Promise<void> { const parsedOpReturn = parseOpReturnData(opReturn ?? '') const paymentId = parsedOpReturn.paymentId - if (paymentId !== undefined && paymentId !== '') { - const clientPayment = await getClientPayment(paymentId) - if (clientPayment !== null) { - if (clientPayment?.amount !== null) { - if (Number(clientPayment?.amount) === Number(txAmount)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - await updateClientPaymentStatus(paymentId, status) - } - } + + // Validate paymentId format (32 hex characters) + if (typeof paymentId !== 'string' || !/^[0-9a-fA-F]{32}$/.test(paymentId)) { + return + } + + try { + const clientPayment = await getClientPayment(paymentId) + if (clientPayment === null) { + return + } + + // Verify transaction address matches payment's registered address + if (clientPayment.address.address !== txAddress) { + console.warn(`Address mismatch for payment ${paymentId}: expected ${clientPayment.address.address}, got ${txAddress}`) + return + } + + // Prevent status downgrades + if (clientPayment.status === 'CONFIRMED' && status === 'ADDED_TO_MEMPOOL') { + return + } + + // Compare amounts using Decimal equality + if (clientPayment.amount !== null) { + const clientAmount = new Prisma.Decimal(clientPayment.amount) + const txAmountDecimal = new Prisma.Decimal(txAmount.toString()) + if (!clientAmount.equals(txAmountDecimal)) { + console.warn(`Payment amount mismatch for ${paymentId}: expected ${clientAmount}, got ${txAmountDecimal}`) + return + } + } + + await updateClientPaymentStatus(paymentId, status) + } catch (error) { + console.error(`Failed to update client payment status for ${paymentId}: ${String(error)}`) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1022-1027)updateClientPaymentStatus(1015-1020)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
services/chronikService.ts (2)
14-17: LGTM! Imports are clear and no name collision.The imported functions
updateClientPaymentStatusandgetClientPaymentare used correctly in the private methodhandleUpdateClientPaymentStatuswithout naming conflicts.
578-580: LGTM! Transaction fetching correctly positioned.Moving the transaction and address fetching logic earlier (before the message type conditionals) is a good refactor that eliminates duplication and makes both the TX_CONFIRMED and TX_ADDED_TO_MEMPOOL paths cleaner.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/unittests/chronikService.test.ts(21 hunks)tests/unittests/handleUpdateClientPaymentStatus.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unittests/chronikService.test.ts (1)
services/chronikService.ts (6)
toHash160(903-911)ChronikBlockchainClient(118-884)multiBlockchainClient(1100-1100)fromHash160(886-901)outputScriptToAddress(914-942)getNullDataScriptData(37-110)
🪛 GitHub Actions: Pull Request Tests
tests/unittests/chronikService.test.ts
[error] 1322-1565: Unit tests in chronikService.test.ts failing: multiple expectations not met (e.g., mockGetClientPayment calls, payment status updates) and chronik fetch-related errors leading to test failures.
|
|
||
| it('Should update payment status when paymentId exists and amounts match', async () => { | ||
| // Mock parseOpReturnData to return a paymentId | ||
| const mockOpReturn = JSON.stringify({ | ||
| paymentId: 'test-payment-id-123', | ||
| message: 'test message' | ||
| }) | ||
|
|
||
| const mockClientPayment = { | ||
| id: '1', | ||
| paymentId: 'test-payment-id-123', | ||
| amount: new Prisma.Decimal('100.00'), | ||
| status: 'PENDING', | ||
| address: { id: '1', address: 'test-address' } | ||
| } | ||
|
|
||
| mockGetClientPayment.mockResolvedValue(mockClientPayment) | ||
| mockUpdateClientPaymentStatus.mockResolvedValue(undefined) | ||
|
|
||
| // Access the private method using bracket notation | ||
| await (client as any).handleUpdateClientPaymentStatus( | ||
| new Prisma.Decimal('100.00'), | ||
| mockOpReturn, | ||
| 'CONFIRMED' | ||
| ) | ||
|
|
||
| expect(mockGetClientPayment).toHaveBeenCalledWith('test-payment-id-123') | ||
| expect(mockUpdateClientPaymentStatus).toHaveBeenCalledWith('test-payment-id-123', 'CONFIRMED') | ||
| }) |
There was a problem hiding this comment.
Payment-ID never extracted from mocked opReturn
All of these new handleUpdateClientPaymentStatus tests build the opReturn payload with JSON.stringify(...). However handleUpdateClientPaymentStatus calls the real parseOpReturnData, which only understands the PAY protocol’s key/value (foo=bar baz=qux) format, not JSON. Consequently parsedOpReturn.paymentId is always undefined, so getClientPayment/updateClientPaymentStatus are never invoked and the expectations fail (as seen in CI). Please switch the mocked opReturn strings to the format that parseOpReturnData actually parses, or stub parseOpReturnData itself. Until then the suite will keep failing.
🤖 Prompt for AI Agents
In tests/unittests/chronikService.test.ts around lines 1296 to 1324, the mocked
opReturn is JSON but the real parseOpReturnData expects PAY protocol key/value
strings (e.g. "paymentId=test-payment-id-123 message=test message"), so
parsedOpReturn.paymentId is undefined and the expectations never run; fix by
either (A) change the mocked opReturn strings to the PAY key=value format that
parseOpReturnData parses (e.g. "paymentId=test-payment-id-123 message=test
message") for these tests, or (B) stub/mock parseOpReturnData to return an
object with paymentId and message for the given opReturn — implement one of
these approaches so getClientPayment and updateClientPaymentStatus are called
with the expected paymentId.
| it('Should update payment status when paymentId exists and amounts match', async () => { | ||
| // Mock parseOpReturnData to return a paymentId | ||
| const mockOpReturn = JSON.stringify({ | ||
| paymentId: 'test-payment-id-123', | ||
| message: 'test message' | ||
| }) | ||
|
|
||
| const mockClientPayment = { | ||
| paymentId: 'test-payment-id-123', | ||
| amount: new Prisma.Decimal('100.00'), | ||
| status: 'PENDING' as ClientPaymentStatus, | ||
| addressString: 'test-address', | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| address: { | ||
| id: '1', | ||
| address: 'test-address', | ||
| networkId: 1, | ||
| syncing: false, | ||
| createdAt: new Date(), | ||
| updatedAt: new Date(), | ||
| lastSynced: null | ||
| } | ||
| } | ||
|
|
||
| mockGetClientPayment.mockResolvedValue(mockClientPayment) | ||
| mockUpdateClientPaymentStatus.mockResolvedValue(undefined) | ||
|
|
||
| // Access the private method using bracket notation | ||
| await (client as any).handleUpdateClientPaymentStatus( | ||
| new Prisma.Decimal('100.00'), | ||
| mockOpReturn, | ||
| 'CONFIRMED' as ClientPaymentStatus | ||
| ) | ||
|
|
||
| expect(mockGetClientPayment).toHaveBeenCalledWith('test-payment-id-123') | ||
| expect(mockUpdateClientPaymentStatus).toHaveBeenCalledWith('test-payment-id-123', 'CONFIRMED') | ||
| }) | ||
|
|
There was a problem hiding this comment.
Tests build impossible opReturn payloads
Here too the mocked opReturn is a JSON string, but handleUpdateClientPaymentStatus runs the real parseOpReturnData, which never returns a paymentId for JSON input. Because parsedOpReturn.paymentId is always undefined, none of the mocked transactionService methods are called and every expectation fails (matching the CI report). Please provide the PAY protocol key/value payload that parseOpReturnData can parse, or mock parseOpReturnData to return the desired structure.
🤖 Prompt for AI Agents
In tests/unittests/handleUpdateClientPaymentStatus.test.ts around lines 86 to
124, the test supplies a JSON string for opReturn but the real parseOpReturnData
used by handleUpdateClientPaymentStatus does not extract paymentId from plain
JSON; update the test to either (a) supply a valid PAY protocol key/value
opReturn string that parseOpReturnData recognizes (including the paymentId key
with the test-payment-id-123 value), or (b) mock parseOpReturnData to return an
object with paymentId: 'test-payment-id-123' (and any other needed fields)
before calling handleUpdateClientPaymentStatus, and keep the existing
transactionService mocks and assertions unchanged.
af461e7 to
22b7370
Compare
lissavxo
force-pushed
the
feat/update-payment-id
branch
from
9f832f2 to
962c953
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
services/transactionService.ts (3)
12-13: Break circular dependency with chronikService.Top-level import of multiBlockchainClient creates a cycle (chronikService → transactionService → chronikService) that can yield undefined bindings at runtime. Lazy import inside the function avoids it.
Based on previous review.
980-1013: Validate and normalize address; guard unknown network; avoid subtle bugs ingeneratePaymentId.
- Use
parseAddressto normalize/validate input before DB ops.- Derive
networkIdfrom the normalized prefix and fail fast if unknown.- Use the normalized address in
addressExistsandconnectOrCreate.Based on previous review.
1015-1020: Add error handling for non-existent payment IDs.The
updateClientPaymentStatusfunction should handle cases where the payment ID doesn't exist.Based on previous review.
services/chronikService.ts (1)
557-578: Fix critical property access error and Prisma Decimal comparison.
- Lines 568, 572:
clientPayment?.addressStringdoes not exist. The correct property path isclientPayment?.address.address(based on the return type fromgetClientPayment).- Line 567: Prisma.Decimal objects must be compared via
.equals(), not by converting toNumber()which can lose precision.- Line 563: No validation that
paymentIdis a valid format (32 hex characters expected).- Missing status downgrade prevention (e.g., CONFIRMED → ADDED_TO_MEMPOOL).
- Missing error handling for
getClientPaymentandupdateClientPaymentStatus.Apply this diff to fix the critical bugs:
private async handleUpdateClientPaymentStatus ( txAmount: string | number | Prisma.Decimal | DecimalJsLike, opReturn: string | undefined, status: ClientPaymentStatus, txAdress: string): Promise<void> { const parsedOpReturn = parseOpReturnData(opReturn ?? '') const paymentId = parsedOpReturn.paymentId - if (paymentId !== undefined && paymentId !== '') { + // Validate paymentId format (32 hex characters for UUID without dashes) + if (typeof paymentId !== 'string' || !/^[0-9a-fA-F]{32}$/.test(paymentId)) { + return + } + + try { const clientPayment = await getClientPayment(paymentId) - if (clientPayment !== null) { - if (clientPayment?.amount !== null) { - if (Number(clientPayment?.amount) === Number(txAmount) && - (clientPayment?.addressString === txAdress)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - if (clientPayment?.addressString === txAdress) { - await updateClientPaymentStatus(paymentId, status) - } - } + if (clientPayment === null) { + return + } + + // Prevent status downgrades + if (clientPayment.status === 'CONFIRMED' && status === 'ADDED_TO_MEMPOOL') { + return } + + // Verify address matches + if (clientPayment.address.address !== txAdress) { + console.warn(`Address mismatch for payment ${paymentId}: expected ${clientPayment.address.address}, got ${txAdress}`) + return + } + + // Check amount if specified + if (clientPayment.amount !== null) { + const clientAmount = new Prisma.Decimal(clientPayment.amount) + const txAmountDecimal = new Prisma.Decimal(txAmount.toString()) + if (!clientAmount.equals(txAmountDecimal)) { + console.warn(`Payment amount mismatch for ${paymentId}: expected ${clientAmount}, got ${txAmountDecimal}`) + return + } + } + + await updateClientPaymentStatus(paymentId, status) + } catch (error) { + console.error(`Failed to update client payment status: ${error}`) } }
🧹 Nitpick comments (2)
services/chronikService.ts (2)
601-604: Consider deduplicating paymentId updates across addresses.While the call to
handleUpdateClientPaymentStatusnow passes the correct parameters, if the same transaction involves multiple addresses with the samepaymentIdin the OP_RETURN, the status update will be attempted multiple times. Consider tracking processedpaymentIdvalues in a Set within the loop to avoid redundant DB calls.
624-625: Consider deduplicating paymentId updates across addresses.Same as the TX_CONFIRMED path above: if the same transaction involves multiple addresses with the same
paymentIdin the OP_RETURN, consider deduplicating to avoid redundant status updates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/chronikService.ts(5 hunks)services/transactionService.ts(3 hunks)tests/unittests/handleUpdateClientPaymentStatus.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unittests/handleUpdateClientPaymentStatus.test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1022-1027)updateClientPaymentStatus(1015-1020)
services/transactionService.ts (3)
constants/index.ts (1)
NETWORK_IDS_FROM_SLUGS(132-135)services/addressService.ts (1)
addressExists(127-141)services/chronikService.ts (1)
multiBlockchainClient(1106-1106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
services/chronikService.ts (2)
604-607: Update call to match fixed method signature.Once you fix the
handleUpdateClientPaymentStatusmethod signature (parametertxAdress→txAddress), update this call site to maintain consistency.Apply this diff after fixing the method signature:
for (const addressWithTransaction of addressesWithTransactions) { const { amount, opReturn } = addressWithTransaction.transaction - await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus, addressWithTransaction.address.address) + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + 'CONFIRMED' as ClientPaymentStatus, + addressWithTransaction.address.address + ) }
627-628: Consistent with TX_CONFIRMED path.This call correctly mirrors the TX_CONFIRMED logic. Consider formatting for readability once the method signature is fixed.
Apply this diff for improved readability:
const { amount, opReturn } = addressWithTransaction.transaction - await this.handleUpdateClientPaymentStatus(amount, opReturn, 'ADDED_TO_MEMPOOL' as ClientPaymentStatus, addressWithTransaction.address.address) + await this.handleUpdateClientPaymentStatus( + amount, + opReturn, + 'ADDED_TO_MEMPOOL' as ClientPaymentStatus, + addressWithTransaction.address.address + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1022-1027)updateClientPaymentStatus(1015-1020)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (1)
services/chronikService.ts (1)
14-17: LGTM on imports.The new imports for client payment functionality are correctly added and match the usage in the implementation.
Also applies to: 18-18, 33-33
| private async handleUpdateClientPaymentStatus ( | ||
| txAmount: string | number | Prisma.Decimal | DecimalJsLike, | ||
| opReturn: string | undefined, status: ClientPaymentStatus, | ||
| txAdress: string): Promise<void> { | ||
| const parsedOpReturn = parseOpReturnData(opReturn ?? '') | ||
| const paymentId = parsedOpReturn.paymentId | ||
| if (paymentId !== undefined && paymentId !== '') { | ||
| const clientPayment = await getClientPayment(paymentId) | ||
| if (clientPayment !== null) { | ||
| if (clientPayment.status === 'CONFIRMED') { | ||
| return | ||
| } | ||
| if (clientPayment?.amount !== null) { | ||
| if (Number(clientPayment?.amount) === Number(txAmount) && | ||
| (clientPayment?.addressString === txAdress)) { | ||
| await updateClientPaymentStatus(paymentId, status) | ||
| } | ||
| } else { | ||
| if (clientPayment?.addressString === txAdress) { | ||
| await updateClientPaymentStatus(paymentId, status) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix typo, property access, Decimal comparison, validation, and error handling.
Several critical issues:
- Line 560: Typo in parameter name
txAdress(missing 'd') — should betxAddress - Lines 571, 575:
clientPayment?.addressStringdoesn't exist. Based ongetClientPaymentreturning{ include: { address: true } }, useclientPayment.address.addressinstead - Line 570: Incorrect Decimal comparison using
Number()equality. Prisma Decimal must be compared via.equals()method - Line 563: Missing paymentId format validation. Should validate against
/^[0-9a-fA-F]{32}$/before DB lookup - Missing error handling: Wrap DB calls in try-catch to handle unknown paymentIds gracefully
Apply this diff:
- private async handleUpdateClientPaymentStatus (
- txAmount: string | number | Prisma.Decimal | DecimalJsLike,
- opReturn: string | undefined, status: ClientPaymentStatus,
- txAdress: string): Promise<void> {
+ private async handleUpdateClientPaymentStatus (
+ txAmount: string | number | Prisma.Decimal | DecimalJsLike,
+ opReturn: string | undefined,
+ status: ClientPaymentStatus,
+ txAddress: string
+ ): Promise<void> {
const parsedOpReturn = parseOpReturnData(opReturn ?? '')
const paymentId = parsedOpReturn.paymentId
- if (paymentId !== undefined && paymentId !== '') {
+
+ // Validate paymentId format (32 hex characters)
+ if (typeof paymentId !== 'string' || !/^[0-9a-fA-F]{32}$/.test(paymentId)) {
+ return
+ }
+
+ try {
const clientPayment = await getClientPayment(paymentId)
- if (clientPayment !== null) {
- if (clientPayment.status === 'CONFIRMED') {
- return
- }
- if (clientPayment?.amount !== null) {
- if (Number(clientPayment?.amount) === Number(txAmount) &&
- (clientPayment?.addressString === txAdress)) {
- await updateClientPaymentStatus(paymentId, status)
- }
- } else {
- if (clientPayment?.addressString === txAdress) {
- await updateClientPaymentStatus(paymentId, status)
- }
- }
+ if (clientPayment === null) {
+ return
+ }
+
+ // Prevent status downgrades
+ if (clientPayment.status === 'CONFIRMED' && status === 'ADDED_TO_MEMPOOL') {
+ return
+ }
+
+ // Verify transaction address matches payment address
+ if (clientPayment.address.address !== txAddress) {
+ return
+ }
+
+ // Check amount if specified
+ if (clientPayment.amount !== null) {
+ const clientAmount = new Prisma.Decimal(clientPayment.amount)
+ const txAmountDecimal = new Prisma.Decimal(txAmount.toString())
+ if (!clientAmount.equals(txAmountDecimal)) {
+ return
+ }
}
+
+ await updateClientPaymentStatus(paymentId, status)
+ } catch (error) {
+ console.error(`Failed to update client payment status: ${String(error)}`)
}
}🤖 Prompt for AI Agents
In services/chronikService.ts around lines 557 to 581, fix several issues:
rename parameter txAdress to txAddress (and update all uses inside the
function); validate parsed paymentId against /^[0-9a-fA-F]{32}$/ before calling
getClientPayment and return early if invalid; wrap the DB calls
(getClientPayment and updateClientPaymentStatus) in a try-catch and handle/log
errors gracefully to avoid unhandled rejections; when checking stored address
use clientPayment.address.address (per include: { address: true }) instead of
clientPayment?.addressString; and compare Prisma Decimal values using the
Decimal equals method (e.g., clientPayment.amount.equals(txAmount) or convert
txAmount to Decimal) rather than Number(...) equality. Ensure the function
returns/short-circuits cleanly on invalid/missing values.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
services/chronikService.ts (1)
557-581: Critical: Fix multiple issues in handleUpdateClientPaymentStatus.Several critical defects:
- Line 562-563: Missing paymentId format validation. Per
generatePaymentId()(UUIDv4 without dashes = 32 hex chars), validate/^[0-9a-fA-F]{32}$/before DB lookup.- Line 564: No try-catch around
getClientPayment— throws on unknown/invalid IDs.- Line 570: Incorrect Prisma Decimal comparison using
Number(). Must use.equals()method or convert both to Decimal and compare.- Lines 571, 575: Property error:
clientPayment.addressStringdoesn't exist. PergetClientPaymentreturn type (include: { address: true }), useclientPayment.address.address.- Line 566-567: Incomplete status downgrade check — only prevents overwriting CONFIRMED, doesn't check if new status is a downgrade.
- Line 572: No error handling for
updateClientPaymentStatus.Apply this diff:
private async handleUpdateClientPaymentStatus ( txAmount: string | number | Prisma.Decimal | DecimalJsLike, - opReturn: string | undefined, status: ClientPaymentStatus, - txAddress: string): Promise<void> { + opReturn: string | undefined, + status: ClientPaymentStatus, + txAddress: string + ): Promise<void> { const parsedOpReturn = parseOpReturnData(opReturn ?? '') const paymentId = parsedOpReturn.paymentId - if (paymentId !== undefined && paymentId !== '') { + + // Validate paymentId format (32 hex characters for UUIDv4 without dashes) + if (typeof paymentId !== 'string' || !/^[0-9a-fA-F]{32}$/.test(paymentId)) { + return + } + + try { const clientPayment = await getClientPayment(paymentId) - if (clientPayment !== null) { - if (clientPayment.status === 'CONFIRMED') { - return - } - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount) && - (clientPayment.addressString === txAddress)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - if (clientPayment.addressString === txAddress) { - await updateClientPaymentStatus(paymentId, status) - } - } + if (clientPayment === null) { + return + } + + // Prevent status downgrades: CONFIRMED > ADDED_TO_MEMPOOL > PENDING + if (clientPayment.status === 'CONFIRMED' && status !== 'CONFIRMED') { + return + } + if (clientPayment.status === 'ADDED_TO_MEMPOOL' && status === 'PENDING') { + return + } + + // Verify transaction address matches payment address + if (clientPayment.address.address !== txAddress) { + return + } + + // Check amount if specified + if (clientPayment.amount !== null) { + const clientAmount = new Prisma.Decimal(clientPayment.amount) + const txAmountDecimal = new Prisma.Decimal(txAmount.toString()) + if (!clientAmount.equals(txAmountDecimal)) { + return + } } + + await updateClientPaymentStatus(paymentId, status) + } catch (error) { + console.error(`${this.CHRONIK_MSG_PREFIX}: Failed to update client payment status for ${paymentId}: ${String(error)}`) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1022-1027)updateClientPaymentStatus(1015-1020)
🔇 Additional comments (1)
services/chronikService.ts (1)
627-628: Same verification needed for ADDED_TO_MEMPOOL path.This call site also looks correct. The fixes to
handleUpdateClientPaymentStatuswill ensure proper status updates for mempool transactions.
| for (const addressWithTransaction of addressesWithTransactions) { | ||
| const { amount, opReturn } = addressWithTransaction.transaction | ||
| await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus, addressWithTransaction.address.address) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify call sites after fixing handleUpdateClientPaymentStatus.
The call to handleUpdateClientPaymentStatus looks correct with proper parameters (amount, opReturn, CONFIRMED status, address). Once the method is fixed per the previous comment, this should work as expected.
After applying the fixes to handleUpdateClientPaymentStatus, verify that:
- CONFIRMED status updates work correctly
- Amount matching validation works with Prisma Decimal
- Address validation prevents mismatched updates
- Errors are logged without crashing the WebSocket handler
🏁 Script executed:
#!/bin/bash
# Verify the handleUpdateClientPaymentStatus usage in TX_CONFIRMED and TX_ADDED_TO_MEMPOOL handlers
rg -n "handleUpdateClientPaymentStatus" services/chronikService.ts -A2 -B2Length of output: 878
🏁 Script executed:
#!/bin/bash
rg -n "private async handleUpdateClientPaymentStatus" -A200 services/chronikService.tsLength of output: 10453
Handle errors & use precise Decimal comparison in handleUpdateClientPaymentStatus
- Wrap both
await updateClientPaymentStatus(...)calls in a try/catch insidehandleUpdateClientPaymentStatus, logging errors rather than letting them bubble up and crash the WebSocket handler. - Replace
Number(clientPayment.amount) === Number(txAmount)with a Decimal‐aware check, e.g.:if (clientPayment.amount.equals(new Prisma.Decimal(txAmount))) { … }
🤖 Prompt for AI Agents
In services/chronikService.ts around lines 604 to 607, the loop calls
handleUpdateClientPaymentStatus which currently lets internal update errors
bubble up and uses Number(...) for amount comparison; modify
handleUpdateClientPaymentStatus to wrap both await
updateClientPaymentStatus(...) calls in a try/catch that logs the error (so the
WebSocket handler doesn't crash) and continues, and change the numeric
comparison to use a Decimal-aware check (e.g., create a Prisma.Decimal from
txAmount and call .equals on clientPayment.amount) instead of
Number(clientPayment.amount) === Number(txAmount).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pages/api/payments/paymentId/index.ts (1)
37-38: Add Allow header to 405 response.The 405 Method Not Allowed response is missing the required
Allowheader that indicates which HTTP methods are supported by this endpoint.Apply this diff to add the Allow header:
} else { + res.setHeader('Allow', ['POST']) res.status(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED_405.statusCode) .json(RESPONSE_MESSAGES.METHOD_NOT_ALLOWED_405) }
🧹 Nitpick comments (2)
pages/api/payments/paymentId/index.ts (2)
9-11: Consider restricting CORS origin for production security.The CORS configuration currently allows all origins by default since no
originoption is specified. If this API endpoint should only be accessed from specific domains, consider adding an explicit origin whitelist.Example with origin restriction:
const cors = Cors({ - methods: ['POST'] + methods: ['POST'], + origin: process.env.NODE_ENV === 'production' + ? ['https://yourdomain.com'] + : true })
13-13: Use proper Next.js types for better type safety.The handler parameters use
anytypes, which bypasses TypeScript's type checking. Consider usingNextApiRequestandNextApiResponsefrom 'next' for improved type safety and IDE support.Apply this diff:
+import type { NextApiRequest, NextApiResponse } from 'next' + -export default async (req: any, res: any): Promise<void> => { +export default async (req: NextApiRequest, res: NextApiResponse): Promise<void> => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pages/api/payments/paymentId/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pages/api/payments/paymentId/index.ts (4)
utils/index.ts (1)
runMiddleware(137-151)utils/validators.ts (2)
parseCreatePaymentIdPOSTRequest(578-590)parseAddress(31-48)services/transactionService.ts (1)
generatePaymentId(980-1013)constants/index.ts (1)
RESPONSE_MESSAGES(8-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (1)
pages/api/payments/paymentId/index.ts (1)
7-7: CORS package version is valid in package.json
The cors dependency is set to ^2.8.5, matching the latest stable release.
| const values = parseCreatePaymentIdPOSTRequest(req.body) | ||
| const address = parseAddress(values.address) | ||
| const amount = values.amount as Decimal | undefined | ||
|
|
||
| const paymentId = await generatePaymentId(address, amount) |
There was a problem hiding this comment.
Fix unsafe type cast for amount parameter.
Line 19 performs an unsafe type cast of values.amount to Decimal | undefined. According to the validator code in utils/validators.ts, parseCreatePaymentIdPOSTRequest returns amount as a string or undefined, not a Decimal. This cast does not perform any conversion and will cause type mismatches when passed to generatePaymentId, which expects Prisma.Decimal.
Apply this diff to properly convert the amount:
+import { Prisma } from '@prisma/client'
+
const values = parseCreatePaymentIdPOSTRequest(req.body)
const address = parseAddress(values.address)
- const amount = values.amount as Decimal | undefined
+ const amount = values.amount !== undefined
+ ? new Prisma.Decimal(values.amount)
+ : undefined
const paymentId = await generatePaymentId(address, amount)📝 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 values = parseCreatePaymentIdPOSTRequest(req.body) | |
| const address = parseAddress(values.address) | |
| const amount = values.amount as Decimal | undefined | |
| const paymentId = await generatePaymentId(address, amount) | |
| import { Prisma } from '@prisma/client' | |
| const values = parseCreatePaymentIdPOSTRequest(req.body) | |
| const address = parseAddress(values.address) | |
| const amount = values.amount !== undefined | |
| ? new Prisma.Decimal(values.amount) | |
| : undefined | |
| const paymentId = await generatePaymentId(address, amount) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
services/chronikService.ts (1)
557-582: Critical issues in handleUpdateClientPaymentStatus: wrong property access, incorrect Decimal comparison, missing validation & error handling.Multiple critical issues remain from previous reviews:
- Lines 572-573, 577:
clientPayment.addressStringdoesn't exist. Based ongetClientPaymentreturn type (includes{ address: true }), useclientPayment.address.address.- Line 572:
Number(clientPayment.amount) === Number(txAmount)is incorrect for Prisma.Decimal. Use.equals()method:clientPayment.amount.equals(new Prisma.Decimal(txAmount)).- Lines 563-564: Missing paymentId format validation. Should validate against
/^[0-9a-fA-F]{32}$/before DB lookup.- Missing try-catch: Wrap
getClientPaymentandupdateClientPaymentStatuscalls to handle unknown paymentIds gracefully without crashing the WebSocket handler.Apply this diff:
private async handleUpdateClientPaymentStatus ( txAmount: string | number | Prisma.Decimal | DecimalJsLike, - opReturn: string | undefined, status: ClientPaymentStatus, - txAddress: string): Promise<void> { + opReturn: string | undefined, + status: ClientPaymentStatus, + txAddress: string + ): Promise<void> { const parsedOpReturn = parseOpReturnData(opReturn ?? '') const paymentId = parsedOpReturn.paymentId - if (paymentId === undefined || paymentId === '') { + + // Validate paymentId format (32 hex characters for UUID without dashes) + if (typeof paymentId !== 'string' || !/^[0-9a-fA-F]{32}$/.test(paymentId)) { return } - const clientPayment = await getClientPayment(paymentId) - if (clientPayment !== null) { - if (clientPayment.status === 'CONFIRMED') { - return - } - if (clientPayment.amount !== null) { - if (Number(clientPayment.amount) === Number(txAmount) && - (clientPayment.addressString === txAddress)) { - await updateClientPaymentStatus(paymentId, status) - } - } else { - if (clientPayment.addressString === txAddress) { - await updateClientPaymentStatus(paymentId, status) - } + + try { + const clientPayment = await getClientPayment(paymentId) + if (clientPayment === null) { + return + } + + // Prevent status downgrades + if (clientPayment.status === 'CONFIRMED' && status === 'ADDED_TO_MEMPOOL') { + return + } + + // Verify transaction address matches payment address + if (clientPayment.address.address !== txAddress) { + return + } + + // Check amount if specified + if (clientPayment.amount !== null) { + const clientAmount = new Prisma.Decimal(clientPayment.amount) + const txAmountDecimal = new Prisma.Decimal(txAmount.toString()) + if (!clientAmount.equals(txAmountDecimal)) { + return + } } + + await updateClientPaymentStatus(paymentId, status) + } catch (error) { + console.error(`Failed to update client payment status for ${paymentId}: ${String(error)}`) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (2)
utils/validators.ts (1)
parseOpReturnData(436-464)services/transactionService.ts (2)
getClientPayment(1022-1027)updateClientPaymentStatus(1015-1020)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
Related to #1021
Depends on
Description
Update Client Payment register when transaction is made
Test plan
Create a payment Id with the endpoint api/payments/paymentId?address=your_address, make a payment with the payment Id created in the op_return, make sure the payment Id register status updates according to transaction status
Summary by CodeRabbit
New Features
Bug Fixes
Tests