Added <outputAddresses> post data variable option #1046
Conversation
…include amounts as well.
…riggers Fix: Added invalid JSON check on payment triggers
fix: ignore undefined fields signature
yarn.lock
Outdated
| bs58check "^3.0.1" | ||
|
|
||
| ecashaddrjs@^2.0.0, "ecashaddrjs@file:../ecashaddrjs": | ||
| ecashaddrjs@^2.0.0, "ecashaddrjs@file:../../../../AppData/Local/Yarn/Cache/v6/npm-chronik-client-cashtokens-3.1.1-rc0-e5e4a3538e8010b70623974a731bf2712506c5e3-integrity/node_modules/ecashaddrjs": |
There was a problem hiding this comment.
This looks very specific to a Windows set up. I was having this file regenerated too, so I think we should do this in a separate PR.
services/chronikService.ts
Outdated
| .sort(([, valueA], [, valueB]) => Number(valueB - valueA)) | ||
| .map(([address]) => address) | ||
| return sortedInputAddresses.map(([address, sats]) => { | ||
| const format = xecaddr.detectAddressFormat(address) |
There was a problem hiding this comment.
This is unnecessary. We already know the network, accessible from this.networkSlug.
Best to define something like decimalCases outside the .map to be either 1e2 or 1e8 based on that, and then here, inside the .map, only apply it, to avoid this redundant checks that could slow the code overall if they are ran many times in parallel (like when a block arrives)
services/chronikService.ts
Outdated
| const sortedOutputAddresses = Array.from(addressSatsMap.entries()) | ||
| .sort(([, valueA], [, valueB]) => Number(valueB - valueA)) | ||
| .map(([address, sats]) => { | ||
| const format = xecaddr.detectAddressFormat(address) |
There was a problem hiding this comment.
Same thing here. No need to detect the network per tx address, it will always be the same one (this.networkSlug)
This reverts commit 5e9f33f.
chedieck
left a comment
chedieck
left a comment
There was a problem hiding this comment.
Just one small requested change & fixing conflicts on the test file.
WalkthroughAdds outputAddresses support across transaction and trigger flows, updates types and constants, revises validators and trigger execution with JSON validation, adjusts UI/help text, and extends broadcast payloads. Updates linting tooling and pre-commit hook. Adds unit tests for transaction and trigger services reflecting new address-amount structures and validation behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Chronik as Chronik Client
participant TxSvc as TransactionService
participant WS as WS Broadcast
Note over Chronik: New: compute input/output address amounts
Chronik->>Chronik: getSortedInputAddresses(tx) -> [{address, amount}]
Chronik->>Chronik: getSortedOutputAddresses(tx) -> [{address, amount}]
Chronik->>TxSvc: getSimplifiedTrasaction(tx, inputAddrs, outputAddrs)
TxSvc-->>Chronik: SimplifiedTransaction { ... inputAddresses[], outputAddresses[] }
Chronik->>WS: broadcastIncomingTx(simplifiedTx)
Note over WS: Downstream consumers receive amounts for inputs/outputs
sequenceDiagram
autonumber
participant Trigger as TriggerService
participant Validator as Validators
participant DB as Prisma.triggerLog
participant HTTP as axios.post
Note over Trigger,Validator: New: JSON validation before POST
Trigger->>Trigger: executeAddressTriggers(tx)
Trigger->>Trigger: reorder outputAddresses (primary first)
Trigger->>Validator: parseTriggerPostData(template, params incl. outputAddresses)
alt JSON valid
Validator-->>Trigger: parsedData
Trigger->>HTTP: POST(url, parsedData, timeout)
HTTP-->>Trigger: response/data or error
Trigger->>DB: create log (success or HTTP error)
else JSON invalid
Validator-->>Trigger: throws SyntaxError
Trigger->>DB: create log (JSON_VALIDATION_ERROR)
Note over Trigger,HTTP: Network request is skipped
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utils/validators.ts (1)
232-246: Fix signature payload for non-strings to avoid “[object Object]” and nondeterminisminputAddresses/outputAddresses currently serialize as “[object Object]”, breaking signature determinism. Normalize by JSON-stringifying arrays and String(...) scalars.
const includedVariables = TRIGGER_POST_VARIABLES.filter(v => postData.includes(v)).sort() - const result = includedVariables.map(varString => { - const key = varString.replace('<', '').replace('>', '') as keyof PostDataParameters - let valueString = '' - if (key === 'opReturn') { - const value = postDataParameters[key] - valueString = `${value.message}+${value.paymentId}` - } else { - valueString = postDataParameters[key] as string - } - return valueString - }).filter(value => value !== undefined && value !== null && value !== '') - return result.join('+') + const result = includedVariables.map(varString => { + const key = varString.replace('<', '').replace('>', '') as keyof PostDataParameters + const v: any = (postDataParameters as any)[key] + switch (key) { + case 'opReturn': + return `${v.message}+${v.paymentId}` + case 'amount': + case 'timestamp': + return String(v) + case 'inputAddresses': + case 'outputAddresses': + return JSON.stringify(v ?? []) + default: + return v as string + } + }).filter(v => v !== undefined && v !== null && v !== '') + return result.join('+')
♻️ Duplicate comments (1)
services/chronikService.ts (1)
390-409: Kudos: network divisor now computed once per method, not per address.This addresses the earlier feedback about redundant checks inside tight loops.
🧹 Nitpick comments (22)
.githooks/pre-commit (1)
2-2: Harden the hook and make yarn quieterExit on errors and use silent yarn to reduce noise.
- yarn lint-staged + set -euo pipefail + yarn -s lint-staged.github/coding-instructions.md (1)
12-12: Fix MD007 (unordered list indentation) and style the directory nameRemoves leading space and wraps directory with backticks.
- - 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.package.json (1)
103-105: Speed up ESLint in lint-stagedEnable cache to make pre-commit faster.
- "node ./node_modules/eslint/bin/eslint.js --fix" + "node ./node_modules/eslint/bin/eslint.js --fix --cache"components/Paybutton/PaybuttonTrigger.tsx (1)
216-216: Clarify variable shape in UIConsider adding a short note: “inputAddresses and outputAddresses are arrays of objects: [{ address, amount }]” to reduce integrator confusion.
utils/validators.ts (1)
276-279: Guard against undefined outputAddresses in template replacementUse a safe default to keep JSON valid when outputAddresses is absent.
- .replace('<outputAddresses>', `${JSON.stringify(postDataParameters.outputAddresses, undefined, 2)}`) + .replace('<outputAddresses>', `${JSON.stringify(postDataParameters.outputAddresses ?? [], undefined, 2)}`)Also consider applying the same
?? []fallback to<inputAddresses>.tests/unittests/triggerService.test.ts (3)
101-123: Prefer spying over reassigning prisma methods.Directly assigning prisma.* to prismaMock methods is brittle. Use jest.spyOn to stub the real instance.
- prismaMock.paybuttonTrigger.findMany.mockResolvedValue([ + jest.spyOn(prisma.paybuttonTrigger, 'findMany').mockResolvedValue([ { id: 'trigger-1', isEmailTrigger: false, postURL: 'https://httpbin.org/post', postData: '{"address": <address>, "outputAddresses": <outputAddresses>}', paybutton: { name: 'My Paybutton', providerUserId: 'user-1' } } as any ]) - prisma.paybuttonTrigger.findMany = prismaMock.paybuttonTrigger.findMany - - prismaMock.paybutton.findFirstOrThrow.mockResolvedValue({ providerUserId: 'user-1' } as any) - prisma.paybutton.findFirstOrThrow = prismaMock.paybutton.findFirstOrThrow - - prismaMock.userProfile.findUniqueOrThrow.mockResolvedValue({ id: 'user-1', preferredCurrencyId: 1 } as any) - prisma.userProfile.findUniqueOrThrow = prismaMock.userProfile.findUniqueOrThrow - - prismaMock.triggerLog.create.mockResolvedValue({} as any) - prisma.triggerLog.create = prismaMock.triggerLog.create + jest.spyOn(prisma.paybutton, 'findFirstOrThrow').mockResolvedValue({ providerUserId: 'user-1' } as any) + jest.spyOn(prisma.userProfile, 'findUniqueOrThrow').mockResolvedValue({ id: 'user-1', preferredCurrencyId: 1 } as any) + jest.spyOn(prisma.triggerLog, 'create').mockResolvedValue({} as any)
88-94: Fix indentation to satisfy ESLint.Align to 4-space indent where flagged.
- expect(result.addr).toBe(primaryAddress) - expect(Array.isArray(result.outs)).toBe(true) - expect(result.outs[0].address).toBe(primaryAddress) - expect(result.outs.map((o: any) => o.address)).toEqual([primaryAddress, other]) - // ensure amounts are present - result.outs.forEach((o: any) => expect(o.amount).toBeDefined()) + expect(result.addr).toBe(primaryAddress) + expect(Array.isArray(result.outs)).toBe(true) + expect(result.outs[0].address).toBe(primaryAddress) + expect(result.outs.map((o: any) => o.address)).toEqual([primaryAddress, other]) + // ensure amounts are present + result.outs.forEach((o: any) => expect(o.amount).toBeDefined()) - expect(Array.isArray(postedBody.outputAddresses)).toBe(true) - expect(postedBody.outputAddresses[0].address).toBe(primaryAddress) - expect(postedBody.outputAddresses.map((o: any) => o.address)).toEqual([primaryAddress, other1, other2]) + expect(Array.isArray(postedBody.outputAddresses)).toBe(true) + expect(postedBody.outputAddresses[0].address).toBe(primaryAddress) + expect(postedBody.outputAddresses.map((o: any) => o.address)).toEqual([primaryAddress, other1, other2])Also applies to: 159-161
96-166: Add one negative-path assertion (optional).Consider asserting prisma.triggerLog.create is called with isError: false for this happy path; add a separate test for case when postData is invalid JSON to ensure JSON validation logs once and no POST occurs.
services/transactionService.ts (1)
44-44: Typo: getSimplifiedTrasaction → getSimplifiedTransaction (non-breaking alias).Export a correctly spelled wrapper and deprecate the old name later.
export function getSimplifiedTrasaction (tx: TransactionWithAddressAndPrices, inputAddresses?: Array<{address: string, amount: Prisma.Decimal}>, outputAddresses?: Array<{address: string, amount: Prisma.Decimal}>): SimplifiedTransaction { @@ return simplifiedTransaction } + +// TODO: deprecate getSimplifiedTrasaction in a future major release. +export function getSimplifiedTransaction ( + tx: TransactionWithAddressAndPrices, + inputAddresses?: Array<{ address: string, amount: Prisma.Decimal }>, + outputAddresses?: Array<{ address: string, amount: Prisma.Decimal }> +): SimplifiedTransaction { + return getSimplifiedTrasaction(tx, inputAddresses, outputAddresses) +}services/triggerService.ts (4)
262-269: Guard against undefined outputAddresses to avoid “undefined” in JSON.If outputAddresses is ever missing, parseTriggerPostData would inject the literal string undefined, breaking JSON. Default to [].
- // We ensure that the primary address (<address> variable) is the first element in the outputAddresses since this is likely more useful for apps using the data than it would be if it was in a random order. - let reorderedOutputAddresses = outputAddresses - if (Array.isArray(outputAddresses)) { - const primary = reorderedOutputAddresses.find(oa => oa.address === address) + // Ensure primary (<address>) is first; default to [] if absent. + let reorderedOutputAddresses = Array.isArray(outputAddresses) ? [...outputAddresses] : [] + if (reorderedOutputAddresses.length > 0) { + const primary = reorderedOutputAddresses.find(oa => oa.address === address) if (primary !== undefined) { reorderedOutputAddresses = [primary, ...reorderedOutputAddresses.filter(o => o.address !== address)] } } @@ - outputAddresses: reorderedOutputAddresses, + outputAddresses: reorderedOutputAddresses,Also applies to: 285-286
216-221: Log types should accept objects, not strings.postedData/responseData are objects in practice. Widen the types to unknown (or JsonValue) to match usage.
-interface PostDataTriggerLog { - postedData: string - postedURL: string - responseData: string -} +interface PostDataTriggerLog { + postedData: unknown + postedURL: string + responseData: unknown +}
408-410: Make address arrays required in PostDataParameters (optional).They are always present on SimplifiedTransaction; making them required tightens the contract and avoids undefined propagation.
- inputAddresses?: Array<{address: string, amount: Prisma.Decimal}> - outputAddresses?: Array<{address: string, amount: Prisma.Decimal}> + inputAddresses: Array<{address: string, amount: Prisma.Decimal}> + outputAddresses: Array<{address: string, amount: Prisma.Decimal}>
448-479: Consider SSRF hardening (optional).If untrusted users can set postURL, add scheme/host allowlisting or blocklink-local checks before axios.post. Keep the timeout as is.
tests/unittests/triggerService2.test.ts (4)
56-71: Prisma triggerLog mock is set but never asserted.If the goal is to ensure validation errors are logged, assert prisma.triggerLog.create call count and payload. Otherwise, remove the unused wiring.
- prisma.triggerLog.create = prismaMock.triggerLog.create + prisma.triggerLog.create = prismaMock.triggerLog.create + // Later in tests that simulate failures: + // expect(prismaMock.triggerLog.create).toHaveBeenCalled()
72-110: Console noise in unit tests.Replace console.log with assertions. If you really need to capture logs, stub them via jest.spyOn(console, 'log') and assert calls.
- console.log('=== Test Case 1: Valid JSON ===') + const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {}) ... - console.log(`Network requests made: ${networkRequestCount}`) + logSpy.mockRestore()
111-153: Use test.each and assert real behavior (or a real consumer).
- forEach inside a single test hides which case fails and doesn’t parallelize well.
- More importantly, you’re still testing your mock.
Refactor to test.each and target the Trigger Service entrypoint so errors propagate to logging and network paths as designed.
-it('should log JSON validation errors with proper details', async () => { - const testCases = [ ... ] - testCases.forEach(({ name, postData, expectedError }) => { - ... - }) -}) +it.each([ + { name: 'Missing closing brace', postData: '{"amount": <amount>, "currency": <currency>', expectedError: /INVALID_DATA_JSON/ }, + { name: 'Invalid property syntax', postData: '{amount: <amount>, "currency": <currency>}', expectedError: /INVALID_DATA_JSON/ }, + { name: 'Extra comma', postData: '{"amount": <amount>,, "currency": <currency>}', expectedError: /INVALID_DATA_JSON/ }, +])('should log JSON validation errors: %s', async ({ postData, expectedError }) => { + // Arrange: call the Trigger Service (preferred) or real parser + // Assert: triggerLog.create called with isError=true, and error matches expectedError +})
1-53: Missing coverage for the new variable (and its shape).Add a test that verifies parseTriggerPostData substitutes correctly and that the consumer (Trigger Service) forwards it in the POST body. Also clarify expected shape:
- PR description says “addresses only”, but code passes address+amount objects. Which is intended?
Let’s align and codify with tests.
Would you like me to push a focused test that exercises executeAddressTriggers end-to-end and asserts POST payload includes outputAddresses as either string[] or Array<{address, amount}>?
Also applies to: 195-236
services/chronikService.ts (5)
390-409: Avoid Number() conversion when sorting BigInt totals.Converting BigInt deltas to Number risks precision loss for very large values. Use direct BigInt comparisons.
- const sortedInputAddresses = Array.from(addressSatsMap.entries()) - .sort(([, valueA], [, valueB]) => Number(valueB - valueA)) + const sortedInputAddresses = Array.from(addressSatsMap.entries()) + .sort(([, a], [, b]) => (a === b ? 0 : a < b ? 1 : -1))
411-431: Duplicate logic between input/output aggregation. Extract a helper to DRY.Both methods aggregate sats by address, sort, and scale by unit divisor. Consider a shared private helper to reduce duplication and cognitive load.
// Outside selected range (supporting snippet) private readonly unitDivisor = this.networkId === XEC_NETWORK_ID ? 1e2 : (this.networkId === BCH_NETWORK_ID ? 1e8 : 1) private aggregateAndSort(io: Array<{outputScript?: string, sats: bigint}>): Array<{address: string, amount: Prisma.Decimal}> { const m = new Map<string, bigint>() for (const x of io) { const addr = outputScriptToAddress(this.networkSlug, x.outputScript) if (addr) m.set(addr, (m.get(addr) ?? 0n) + x.sats) } return Array.from(m.entries()) .sort(([, a], [, b]) => (a === b ? 0 : a < b ? 1 : -1)) .map(([address, sats]) => ({ address, amount: new Prisma.Decimal(sats.toString()).dividedBy(this.unitDivisor) })) }And then:
- private getSortedOutputAddresses (transaction: Tx): Array<{address: string, amount: Prisma.Decimal}> { - const addressSatsMap = new Map<string, bigint>() - transaction.outputs.forEach((out) => { - const address = outputScriptToAddress(this.networkSlug, out.outputScript) - if (address !== undefined && address !== '') { - const currentValue = addressSatsMap.get(address) ?? 0n - addressSatsMap.set(address, currentValue + out.sats) - } - }) - const unitDivisor = this.networkId === XEC_NETWORK_ID - ? 1e2 - : (this.networkId === BCH_NETWORK_ID ? 1e8 : 1) - const sortedOutputAddresses = Array.from(addressSatsMap.entries()) - .sort(([, valueA], [, valueB]) => Number(valueB - valueA)) - .map(([address, sats]) => { - const decimal = new Prisma.Decimal(sats.toString()) - const amount = decimal.dividedBy(unitDivisor) - return { address, amount } - }) - return sortedOutputAddresses - } + private getSortedOutputAddresses (transaction: Tx): Array<{address: string, amount: Prisma.Decimal}> { + return this.aggregateAndSort(transaction.outputs) + }
420-430: Same BigInt sort issue applies here.Mirror the comparator fix from inputs to outputs.
- .sort(([, valueA], [, valueB]) => Number(valueB - valueA)) + .sort(([, a], [, b]) => (a === b ? 0 : a < b ? 1 : -1))
399-401: Minor: centralize unit divisor once per instance.Compute a readonly unitDivisor in the constructor (after networkId assignment) to avoid re-deriving in hot paths.
// Outside selected range (supporting snippet) private readonly unitDivisor: number constructor (networkSlug: string) { ... this.networkId = NETWORK_IDS_FROM_SLUGS[networkSlug] this.unitDivisor = this.networkId === XEC_NETWORK_ID ? 1e2 : (this.networkId === BCH_NETWORK_ID ? 1e8 : 1) ... }And replace local unitDivisor usages with this.unitDivisor.
Also applies to: 420-422
477-486: PR description vs implementation mismatch on .PR text says “addresses only; not amounts,” but code surfaces {address, amount}. Either adjust code to emit addresses only (if that’s the requirement) or update PR description and user-facing docs to reflect the inclusion of amounts.
If “addresses only” is desired for post data:
- .replace('<outputAddresses>', `${JSON.stringify(postDataParameters.outputAddresses, undefined, 2)}`) + .replace('<outputAddresses>', `${JSON.stringify(postDataParameters.outputAddresses.map(o => o.address), undefined, 2)}`)(in utils/validators.ts)
Also applies to: 505-517
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (13)
.githooks/pre-commit(1 hunks).github/coding-instructions.md(1 hunks)components/Paybutton/PaybuttonTrigger.tsx(1 hunks)constants/index.ts(1 hunks)package.json(2 hunks)services/chronikService.ts(4 hunks)services/transactionService.ts(2 hunks)services/triggerService.ts(4 hunks)tests/unittests/transactionService.test.ts(1 hunks)tests/unittests/triggerService.test.ts(1 hunks)tests/unittests/triggerService2.test.ts(1 hunks)utils/validators.ts(4 hunks)ws-service/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/unittests/transactionService.test.ts (1)
tests/mockedObjects.ts (1)
mockedTransaction(400-422)
tests/unittests/triggerService.test.ts (3)
utils/validators.ts (1)
parseTriggerPostData(262-289)prisma-local/mockedClient.ts (1)
prismaMock(19-19)services/triggerService.ts (1)
executeAddressTriggers(234-315)
tests/unittests/triggerService2.test.ts (2)
utils/validators.ts (1)
parseTriggerPostData(262-289)prisma-local/mockedClient.ts (1)
prismaMock(19-19)
services/transactionService.ts (1)
ws-service/types.ts (1)
SimplifiedTransaction(11-34)
services/triggerService.ts (1)
utils/validators.ts (1)
parseTriggerPostData(262-289)
services/chronikService.ts (3)
constants/index.ts (2)
XEC_NETWORK_ID(157-157)BCH_NETWORK_ID(158-158)services/transactionService.ts (3)
upsertTransaction(358-390)TransactionWithAddressAndPrices(104-104)getSimplifiedTrasaction(44-71)ws-service/types.ts (1)
BroadcastTxData(5-9)
🪛 markdownlint-cli2 (0.17.2)
.github/coding-instructions.md
12-12: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 ESLint
tests/unittests/triggerService.test.ts
[error] 88-88: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 89-89: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 90-90: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 91-91: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 92-92: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 93-93: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 159-159: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 160-160: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
[error] 161-161: Expected indentation of 4 spaces but found 2.
(@typescript-eslint/indent)
🔇 Additional comments (10)
constants/index.ts (1)
250-251: Confirm semantics of (addresses-only vs address+amount objects)PR description says “addresses only,” but types/tests elsewhere use
{ address, amount }. Please confirm and align docs/UI accordingly.tests/unittests/transactionService.test.ts (1)
170-193: LGTM: validates passthrough of input/output address objectsSolid coverage ensuring getSimplifiedTrasaction returns provided arrays unchanged.
tests/unittests/triggerService.test.ts (2)
61-94: Good coverage for parsing and primary-first invariant.Assertions correctly enforce placeholder replacement, primary-first ordering, and presence of amounts.
96-166: End-to-end trigger execution test looks solid.Mocks isolate network/DB, and the expectations verify ordering and amounts on the posted body.
services/transactionService.ts (1)
44-67: Pass-through of outputAddresses is correct and keeps defaults sane.Extending getSimplifiedTrasaction to accept and forward outputAddresses mirrors inputAddresses handling and preserves [] defaults.
services/triggerService.ts (2)
262-269: Primary-first reordering looks good and stable.Uses find/filter to keep non-primary order intact.
418-445: Good JSON validation branch before network I/O.Early-exit on invalid JSON prevents unnecessary POSTs and records useful diagnostics.
services/chronikService.ts (3)
477-486: Behavior change: broadcasting now includes both inputAddresses and outputAddresses with amounts.
- Good: computed once per tx and reused for all affected addresses.
- Verify: downstream consumers (WS clients, triggers) expect the new fields and the amount type (Prisma.Decimal serializes to string). If clients previously expected address-only arrays, this is a breaking change.
Would you confirm whether UI/WS consumers have been updated to read outputAddresses and handle amount as string?
505-517: WS payload surface changed; ensure backward compatibility and docs.getSimplifiedTrasaction now embeds outputAddresses. Please:
- Update WS contract docs/changelog.
- Consider feature flagging or versioning if any consumer expects the previous shape.
- Confirm Prisma.Decimal JSON shape is acceptable on the wire (string).
533-544: Same WS payload considerations for block-sync path.Mirror the compatibility checks for the block path as for mempool path.
| jest.mock('../../utils/validators', () => { | ||
| const originalModule = jest.requireActual('../../utils/validators') | ||
| return { | ||
| ...originalModule, | ||
| parseTriggerPostData: jest.fn() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests are mocking the SUT (parseTriggerPostData) and then asserting on the mock — tautological.
These tests never exercise real parsing or any consumer of it. Either:
- Stop mocking parseTriggerPostData and test its real behavior; or
- Keep the mock, but test the Trigger Service (e.g., executeAddressTriggers) that consumes it.
Right now they provide no signal and can pass while the real flow is broken.
Apply this diff to stop mocking validators and prepare to spy instead:
-jest.mock('../../utils/validators', () => {
- const originalModule = jest.requireActual('../../utils/validators')
- return {
- ...originalModule,
- parseTriggerPostData: jest.fn()
- }
-})
-const mockedParseTriggerPostData = parseTriggerPostData as jest.MockedFunction<typeof parseTriggerPostData>
+// Use real validators; you can spy on specific exports if needed
+import * as validators from '../../utils/validators'
+const mockedParseTriggerPostData = jest.spyOn(validators, 'parseTriggerPostData')📝 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.
| jest.mock('../../utils/validators', () => { | |
| const originalModule = jest.requireActual('../../utils/validators') | |
| return { | |
| ...originalModule, | |
| parseTriggerPostData: jest.fn() | |
| } | |
| }) | |
| // Use real validators; you can spy on specific exports if needed | |
| import * as validators from '../../utils/validators' | |
| const mockedParseTriggerPostData = jest.spyOn(validators, 'parseTriggerPostData') |
🤖 Prompt for AI Agents
In tests/unittests/triggerService2.test.ts around lines 10 to 16, the test
currently fully mocks parseTriggerPostData which makes assertions tautological;
instead remove the jest.mock override for parseTriggerPostData and replace it
with a spy on the real module so you exercise real parsing while still allowing
inspection. Concretely: stop mocking '../../utils/validators' entirely (or at
least stop replacing parseTriggerPostData), import the real validators module,
use jest.spyOn(validators, 'parseTriggerPostData') in each test to observe
calls/return values, and restore the spy after each test (jest.restoreAllMocks
or spy.mockRestore) so tests assert real behavior or the consumer flow rather
than a stub.
| 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() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
“Reject invalid JSON” test only proves the mock throws.
If you want to validate error mapping, call the real parseTriggerPostData with invalid JSON and assert the returned 400 message shape; if you want to validate Trigger Service behavior, invoke the service and assert that no network call happens and errors are logged.
As-is, this test provides no coverage of actual logic.
🤖 Prompt for AI Agents
In tests/unittests/triggerService2.test.ts around lines 21 to 37, the current
test only proves the mock throws and doesn't exercise real error mapping or
service behavior; update the test to either (A) call the real
parseTriggerPostData with the invalidPostData string (do not mock) and assert it
returns/throws the mapped 400 error shape/message expected by your API layer, or
(B) test the Trigger Service path by invoking the service method with the
invalid JSON, asserting no outbound network calls were made (mock network client
and assert not called) and that the error logger was invoked with the expected
message; remove the throw-only mock-based assertion and replace it with the
appropriate real-call assertions plus verification of mocks for network and
logging as described.
| 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.
🛠️ Refactor suggestion
“Allow valid JSON” test also only asserts on your mock.
Either assert the real parser output (with concrete postDataParameters), or call the Trigger Service and assert axios.post payload. Current version is non-actionable coverage.
🤖 Prompt for AI Agents
In tests/unittests/triggerService2.test.ts around lines 39 to 53, the "Allow
valid JSON" test only asserts that the mocked parser was called instead of
validating real behavior; replace the current mock-based assertion with a
concrete assertion of the parser or an integration assertion: either remove
mockedParseTriggerPostData.mockReturnValue and call parseTriggerPostData with a
real JSON string (e.g. '{"amount":100,"currency":"XEC"}') and appropriate
postDataParameters, then expect the returned value toEqual the
expectedParsedData, or call the Trigger Service method that sends the request
(mock axios) and assert axios.post was called with the parsed payload (amount
and currency) so the test verifies actual parsing and payload composition rather
than only the mock invocation.
| it('should handle edge cases gracefully', async () => { | ||
| const edgeCases = [ | ||
| { | ||
| name: 'Empty post data', | ||
| postData: '', | ||
| mockError: new Error('No data to parse') | ||
| }, | ||
| { | ||
| name: 'Null-like post data', | ||
| postData: 'null', | ||
| mockError: new Error('Invalid null data') | ||
| }, | ||
| { | ||
| name: 'Non-object JSON', | ||
| postData: '"just a string"', | ||
| mockError: new Error('Expected object') | ||
| } | ||
| ] | ||
|
|
||
| edgeCases.forEach(({ name, postData, mockError }) => { | ||
| console.log(`\n=== Testing edge case: ${name} ===`) | ||
|
|
||
| mockedParseTriggerPostData.mockImplementation(() => { | ||
| throw mockError | ||
| }) | ||
|
|
||
| try { | ||
| parseTriggerPostData({ | ||
| userId: 'user-123', | ||
| postData, | ||
| postDataParameters: {} as any | ||
| }) | ||
| console.log('❌ Should have failed') | ||
| } catch (error) { | ||
| console.log('✅ Handled gracefully:', (error as Error).message) | ||
| } | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Edge-case tests still mock the parser and don’t hit real code paths.
Either:
- Keep the mock but exercise the Trigger Service to verify logging/short-circuit; or
- Exercise the real parser with representative inputs and assert mapped error messages.
Current shape gives a false sense of coverage.
| describe('Performance and Efficiency Benefits', () => { | ||
| it('should demonstrate network request avoidance', async () => { | ||
| let networkRequestCount = 0 | ||
|
|
||
| mockedAxios.post.mockImplementation(async () => { | ||
| networkRequestCount++ | ||
| return { data: { success: true } } | ||
| }) | ||
|
|
||
| console.log('\n=== Performance Test: Valid vs Invalid JSON ===') | ||
|
|
||
| mockedParseTriggerPostData.mockReturnValue({ amount: 100 }) | ||
|
|
||
| try { | ||
| parseTriggerPostData({ | ||
| userId: 'user-123', | ||
| postData: '{"amount": <amount>}', | ||
| postDataParameters: {} as any | ||
| }) | ||
| await mockedAxios.post('https://example.com', { amount: 100 }) | ||
| console.log('✅ Valid JSON: Network request made') | ||
| } catch (error) { | ||
| console.log('❌ Unexpected error with valid JSON') | ||
| } | ||
|
|
||
| mockedParseTriggerPostData.mockImplementation(() => { | ||
| throw new SyntaxError('Invalid JSON') | ||
| }) | ||
|
|
||
| try { | ||
| parseTriggerPostData({ | ||
| userId: 'user-123', | ||
| postData: '{"amount": <amount>', | ||
| postDataParameters: {} as any | ||
| }) | ||
| } catch (error) { | ||
| console.log('✅ Invalid JSON: No network request made') | ||
| } | ||
|
|
||
| console.log(`Network requests made: ${networkRequestCount}`) | ||
| expect(networkRequestCount).toBe(1) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
“Performance” test is not meaningful.
Manually calling axios.post after a mocked-OK parse doesn’t validate any real performance behavior or request avoidance. You should:
- Invoke the Trigger Service with valid/invalid JSON.
- Assert mockedAxios.post call count (1 vs 0).
Otherwise, delete this test.
🤖 Prompt for AI Agents
In tests/unittests/triggerService2.test.ts around lines 195 to 236, the
"Performance" test is invalid because it manually calls mockedAxios.post after
stubbing parse instead of exercising the Trigger Service; replace the manual
axios.post invocation with calls to the Trigger Service entry point (passing
valid and invalid postData), assert mockedAxios.post was called exactly once for
the valid case and not at all for the invalid case, and remove the console
logs—alternatively delete the whole test if you do not want to exercise the
service.
| inputAddresses: Array<{ | ||
| address: string | ||
| amount: Prisma.Decimal | ||
| }> | ||
| outputAddresses: Array<{ | ||
| address: string | ||
| amount: Prisma.Decimal | ||
| }> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all producers updated and JSON serialization is stable
- Ensure every call to build/broadcast SimplifiedTransaction now sets both inputAddresses and outputAddresses.
- Confirm Prisma.Decimal stringifies as expected over WS (no consumer regressions).
Run to find call sites still passing < 3 args to getSimplifiedTrasaction:
🏁 Script executed:
#!/bin/bash
# Calls with 1 or 2 args (should be none)
ast-grep --pattern $'getSimplifiedTrasaction($_)' || true
ast-grep --pattern $'getSimplifiedTrasaction($_, $_)' || true
# Expected usage (3 args)
ast-grep --pattern $'getSimplifiedTrasaction($_, $_, $_)' -nLength of output: 275
Ensure all getSimplifiedTrasaction calls include both address arrays & verify Decimal serialization
- services/transactionService.ts:36 still invokes
getSimplifiedTrasaction(tx)with only the transaction object; update this call to pass the newinputAddressesandoutputAddressesarguments. - Manually confirm that
Prisma.Decimalvalues stringify over your WS layer as expected (e.g. as strings) to avoid breaking consumers.
🤖 Prompt for AI Agents
In ws-service/types.ts around lines 20 to 27 and services/transactionService.ts
at the call site around line 36, update the invocation of
getSimplifiedTrasaction to pass the two new arrays (inputAddresses and
outputAddresses) along with the tx object, making sure the function signature
matches; additionally, ensure Prisma.Decimal values are serialized across the
WebSocket layer (e.g., convert Decimal fields to strings via .toString() or map
them to primitive strings before sending) so consumers receive stable
JSON-friendly types.
Description
With this, it's possible to get the output addresses for a transaction. We currently only list the addresses themselves and not the amount sent to each address.
Test plan
Summary by CodeRabbit
New Features
Documentation
Tests
Chores