Feature/issue 52 database backed event ledger#266
Open
Danitello123 wants to merge 4 commits into
Open
Conversation
added 4 commits
June 26, 2026 12:58
📝 WalkthroughWalkthroughThree backend services gain new persistence and event-processing capabilities: the payments service adds a Sequelize-backed ChangesPayments: Soroban Transaction Ledger
Orchestrator: DB-backed Workflow Persistence
Notifications: Escrow Event Listener
Migration Runner
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 13
🧹 Nitpick comments (1)
apps/backend/notifications/src/escrowListener.test.ts (1)
64-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise at least two polls in this test.
Because
checkAndMarkDispatchedis mocked to always returntrueand the test never asserts call counts, this only proves the happy path. It will still pass if the same event is sent again on the next poll. Drive a second poll and assert duplicate suppression so regressions in the idempotency path are caught.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/notifications/src/escrowListener.test.ts` around lines 64 - 108, The Escrow listener test only covers a single happy-path poll, so it does not verify idempotency across repeated event fetches. Update the test around startEscrowEventListener and SorobanRpc.Server.prototype.getEvents to exercise at least two polling cycles, then assert checkAndMarkDispatched is not reprocessed for the same escrow event and that email.sendEmail is only triggered once. Use the existing mocks and event fixture in escrowListener.test.ts to drive the duplicate suppression path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/notifications/README.md`:
- Around line 13-21: The environment variable checklist for the notifications
docs is missing the VAPID keys required by the push notification path. Update
the list in the README section that documents deployment/configuration for the
notifications service to include VAPID_PUBLIC_KEY and VAPID_PRIVATE_KEY, using
the existing env-var naming style alongside sendPushNotification and
dispatchEscrowNotification-related settings.
In `@apps/backend/notifications/src/escrowListener.ts`:
- Around line 57-67: The polling logic in escrowListener is advancing the
checkpoint too early, which can skip unprocessed events when getEvents returns a
partial batch or when parse/dispatch errors are swallowed. Update the event loop
in escrowListener to paginate through all results from server.getEvents, and
only persist latestLedger after successfully processing up to the highest
ledger/event handled without failure. Use the existing escrowListener polling
flow and the latestLedger checkpoint write to ensure the checkpoint reflects
only completed work.
- Around line 193-214: The idempotency flow in `checkAndMarkDispatched`
currently marks the notification as dispatched before `sendEmail` or
`sendPushNotification` succeeds, so failures in `escrowListener` can block all
retries. Update the `escrowListener` send paths to use a temporary in-flight
lock or to clear the Redis marker when `sendEmail`/`sendPushNotification` fails,
and only persist the final dispatched marker after a successful provider call.
Apply this to both the email and push branches so retries remain possible after
transient config/provider errors.
- Around line 47-54: The initial backfill in escrowListener’s checkpoint logic
can produce a non-positive start ledger when there is no stored checkpoint and
latestLedger is below 100. Update the startLedger initialization in
escrowListener so it is clamped to a valid minimum before the first RPC query,
while keeping the existing latestLedger upper bound check intact. Use the
existing lastProcessedKey, lastProcessedStr, and startLedger flow to make the
fix in the same block.
In `@apps/backend/orchestrator/src/index.ts`:
- Around line 95-125: The startup recovery flow in index.ts should not continue
to startHttpServer() after a recovery failure. Update the try/catch around the
pool.query and restorePurchaseWorkflow loop so that a failure either aborts
startup by rethrowing/failing fast, or you isolate row-level errors inside the
loop and only proceed when recovery completes successfully. Make the behavior
clear in the startup path around startHttpServer, getPool, and
restorePurchaseWorkflow.
- Around line 33-64: persistWorkflowState currently writes to purchase_workflows
but never updates context._dbVersion after a successful save, so callers keep
using stale version state. Update the persistWorkflowState flow to return the
new version from the INSERT/UPDATE SQL in both branches, then assign that
version back onto context._dbVersion before returning; use the existing
persistWorkflowState and expectedVersion logic to keep the optimistic
concurrency path consistent.
In `@apps/backend/payments/events/index.ts`:
- Around line 201-211: The submission write path in
SorobanTransactionLedger.findOrCreate is still race-prone because it relies on a
read-then-insert pattern. Replace this with an atomic upsert/insert-or-ignore
approach around the hash key so concurrent submissions reuse the same row
instead of colliding. Update the ledger write logic in the submission flow to
ensure idempotent behavior without primary key failures.
- Around line 252-258: The status transition logic in the ledger update path
leaves confirmedAt populated when a record moves from CONFIRMED to FAILED.
Update the conditional handling in the ledgerEntry status update block so that
when the new status is not CONFIRMED, confirmedAt is explicitly cleared along
with setting errorDetails, keeping the ledgerEntry state consistent.
- Around line 213-219: The retry path in the transaction submission update block
leaves terminal data behind when a row is moved back to PENDING. In the existing
ledgerEntry update logic inside the submission/retry flow, clear any
terminal-only fields such as confirmedAt and errorDetails whenever status is
reset to PENDING, alongside updating method/orderId/contractId. Keep the fix
localized to the same created/ledgerEntry branch so a previously CONFIRMED or
FAILED row no longer appears terminal during reconciliation.
In `@apps/backend/payments/src/db.ts`:
- Around line 11-13: The Sequelize pool config in db.ts should validate
DATABASE_POOL_MIN and DATABASE_POOL_MAX before constructing the pool settings
instead of blindly using Number(...). Add explicit parsing/validation so
non-numeric values are rejected and ensure the resolved min is not greater than
max, then fail fast with a clear config error before creating the Sequelize
instance.
In `@database/migrations/005_purchase_workflows.sql`:
- Around line 1-8: The purchase_workflows table currently allows any string in
state, but the orchestrator code in apps/backend/orchestrator/src/index.ts
treats it as PurchaseState and relies on a fixed set of valid terminal states.
Update the migration for purchase_workflows to constrain state with a CHECK
constraint or enum that matches the workflow-state contract used by
PurchaseState and the terminal-state logic, so only valid states can be
persisted or restored.
In `@scripts/setup/migrate.js`:
- Around line 34-42: The migration retry handling in migrate.js is too broad
because the catch around client.query(migrationSql) treats any "already exists"
error as a successful file-level skip, which can hide partially applied SQL
batches. Update the migration flow so duplicate-object errors are handled per
statement or otherwise detected before executing the batch, and do not mark the
whole migration file as applied when only part of it ran. Use the existing
migrate script logic and the client.query/migrationSql execution path to locate
the fix, and ensure files like 002_gateway_auth_limits.sql can be safely rerun
without suppressing later DDL.
In `@tests/unit/src/payments-ledger.test.js`:
- Around line 45-74: The payments ledger tests are order-dependent because
`updateLedgerStatus()` is exercised against state created by the previous
`logSubmission()` test. Add per-test setup/teardown in `payments-ledger.test.js`
so each `it(...)` creates its own submission record before calling
`updateLedgerStatus`, and keep the `logSubmission`/`updateLedgerStatus`
assertions isolated. Use the existing helpers directly in each test so
`CONFIRMED`, `FAILED`, and any retry-to-`PENDING` cases validate independently
of test execution order.
---
Nitpick comments:
In `@apps/backend/notifications/src/escrowListener.test.ts`:
- Around line 64-108: The Escrow listener test only covers a single happy-path
poll, so it does not verify idempotency across repeated event fetches. Update
the test around startEscrowEventListener and
SorobanRpc.Server.prototype.getEvents to exercise at least two polling cycles,
then assert checkAndMarkDispatched is not reprocessed for the same escrow event
and that email.sendEmail is only triggered once. Use the existing mocks and
event fixture in escrowListener.test.ts to drive the duplicate suppression path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 54df3ec0-e213-4954-8c80-20cd71d7b259
📒 Files selected for processing (15)
apps/backend/notifications/README.mdapps/backend/notifications/src/escrowListener.test.tsapps/backend/notifications/src/escrowListener.tsapps/backend/notifications/src/index.tsapps/backend/orchestrator/src/index.test.tsapps/backend/orchestrator/src/index.tsapps/backend/payments/README.mdapps/backend/payments/events/index.tsapps/backend/payments/package.jsonapps/backend/payments/src/db.tsapps/backend/payments/src/models/SorobanTransactionLedger.tsdatabase/migrations/004_soroban_transaction_ledger.sqldatabase/migrations/005_purchase_workflows.sqlscripts/setup/migrate.jstests/unit/src/payments-ledger.test.js
Comment on lines
+13
to
+21
| ## Environment Variables | ||
|
|
||
| - `ESCROW_CONTRACT_ID`: The Soroban contract ID for the escrow contract, to listen for events. | ||
| - `SOROBAN_RPC_URL`: The Stellar RPC URL to poll for on-chain events. | ||
| - `REDIS_URL`: Redis connection URL, used for worker idempotency and deduplication. | ||
| - `DATABASE_URL`: PostgreSQL connection URL, used for wallet lookup adapter. | ||
| - `SENDGRID_API_KEY`: SendGrid API key for emails. | ||
| - `FROM_EMAIL`: Sender email address. | ||
| - `LOG_LEVEL`: Log level (e.g. info, debug). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the VAPID keys used by the push path.
dispatchEscrowNotification can send push notifications, and sendPushNotification fails when VAPID_PUBLIC_KEY / VAPID_PRIVATE_KEY are unset. This new env-var checklist should include them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/notifications/README.md` around lines 13 - 21, The environment
variable checklist for the notifications docs is missing the VAPID keys required
by the push notification path. Update the list in the README section that
documents deployment/configuration for the notifications service to include
VAPID_PUBLIC_KEY and VAPID_PRIVATE_KEY, using the existing env-var naming style
alongside sendPushNotification and dispatchEscrowNotification-related settings.
Comment on lines
+47
to
+54
| const lastProcessedKey = `escrow_listener:last_ledger:${contractId}`; | ||
| const lastProcessedStr = await redis.get(lastProcessedKey); | ||
|
|
||
| let startLedger = lastProcessedStr ? parseInt(lastProcessedStr, 10) + 1 : latestLedger - 100; | ||
|
|
||
| if (startLedger > latestLedger) { | ||
| startLedger = latestLedger; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clamp the initial backfill ledger to a valid minimum.
When there is no checkpoint and latestLedger < 100, Line 50 computes startLedger <= 0. That produces an impossible ledger range for the first RPC query on fresh/local networks.
💡 Small fix
- let startLedger = lastProcessedStr ? parseInt(lastProcessedStr, 10) + 1 : latestLedger - 100;
+ let startLedger = lastProcessedStr
+ ? parseInt(lastProcessedStr, 10) + 1
+ : Math.max(1, latestLedger - 100);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const lastProcessedKey = `escrow_listener:last_ledger:${contractId}`; | |
| const lastProcessedStr = await redis.get(lastProcessedKey); | |
| let startLedger = lastProcessedStr ? parseInt(lastProcessedStr, 10) + 1 : latestLedger - 100; | |
| if (startLedger > latestLedger) { | |
| startLedger = latestLedger; | |
| } | |
| const lastProcessedKey = `escrow_listener:last_ledger:${contractId}`; | |
| const lastProcessedStr = await redis.get(lastProcessedKey); | |
| let startLedger = lastProcessedStr | |
| ? parseInt(lastProcessedStr, 10) + 1 | |
| : Math.max(1, latestLedger - 100); | |
| if (startLedger > latestLedger) { | |
| startLedger = latestLedger; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/notifications/src/escrowListener.ts` around lines 47 - 54, The
initial backfill in escrowListener’s checkpoint logic can produce a non-positive
start ledger when there is no stored checkpoint and latestLedger is below 100.
Update the startLedger initialization in escrowListener so it is clamped to a
valid minimum before the first RPC query, while keeping the existing
latestLedger upper bound check intact. Use the existing lastProcessedKey,
lastProcessedStr, and startLedger flow to make the fix in the same block.
Comment on lines
+57
to
+67
| const eventsResponse = await server.getEvents({ | ||
| startLedger, | ||
| filters: [ | ||
| { | ||
| type: "contract", | ||
| contractIds: [contractId], | ||
| topics: [["*"]], // Match any topics for this contract | ||
| }, | ||
| ], | ||
| limit: 1000, | ||
| }); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't checkpoint latestLedger after a partial batch.
Lines 57-67 fetch at most 1000 events, but Line 124 always stores latestLedger. Any backlog larger than that, or any parse/dispatch failure swallowed on Lines 119-121, will be skipped permanently on the next poll. Advance the checkpoint only to the highest ledger/event that finished successfully, and keep paging until the batch is exhausted.
Also applies to: 74-125
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/notifications/src/escrowListener.ts` around lines 57 - 67, The
polling logic in escrowListener is advancing the checkpoint too early, which can
skip unprocessed events when getEvents returns a partial batch or when
parse/dispatch errors are swallowed. Update the event loop in escrowListener to
paginate through all results from server.getEvents, and only persist
latestLedger after successfully processing up to the highest ledger/event
handled without failure. Use the existing escrowListener polling flow and the
latestLedger checkpoint write to ensure the checkpoint reflects only completed
work.
Comment on lines
+193
to
+214
| const shouldSend = await checkAndMarkDispatched(redis, { | ||
| userId: target.userId, | ||
| channel: "email", | ||
| eventType: event.eventType, | ||
| eventId, | ||
| }); | ||
|
|
||
| if (shouldSend) { | ||
| tasks.push( | ||
| sendEmail({ | ||
| to: target.email, | ||
| subject, | ||
| templateName, | ||
| templateData: { | ||
| orderId: event.orderId, | ||
| amount: event.amountStroops, | ||
| }, | ||
| }).catch((err) => | ||
| log.error("Failed to send escrow email", { error: err, userId: target.userId }) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Mark notifications as dispatched only after the provider call succeeds.
checkAndMarkDispatched in apps/backend/notifications/src/idempotency.ts:22-29 writes the Redis key before sendEmail / sendPushNotification run, and those helpers can fail on config/provider errors (apps/backend/notifications/email/index.ts:38-51, apps/backend/notifications/push/index.ts:25-33). A transient failure therefore suppresses every future retry for that event. Use a short-lived in-flight lock plus a separate success marker, or delete the key on send failure.
Also applies to: 219-247
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/notifications/src/escrowListener.ts` around lines 193 - 214, The
idempotency flow in `checkAndMarkDispatched` currently marks the notification as
dispatched before `sendEmail` or `sendPushNotification` succeeds, so failures in
`escrowListener` can block all retries. Update the `escrowListener` send paths
to use a temporary in-flight lock or to clear the Redis marker when
`sendEmail`/`sendPushNotification` fails, and only persist the final dispatched
marker after a successful provider call. Apply this to both the email and push
branches so retries remain possible after transient config/provider errors.
Comment on lines
+33
to
+64
| /** Saves current machine state and context. */ | ||
| export async function persistWorkflowState( | ||
| orderId: string, | ||
| state: string, | ||
| context: Record<string, any> | ||
| ): Promise<void> { | ||
| const pool = getPool(); | ||
| const userId = context.userId; | ||
| const expectedVersion = context._dbVersion as number | undefined; | ||
|
|
||
| if (expectedVersion !== undefined) { | ||
| const res = await pool.query( | ||
| `UPDATE purchase_workflows | ||
| SET state = $1, context = $2, version = version + 1, updated_at = CURRENT_TIMESTAMP | ||
| WHERE order_id = $3 AND version = $4`, | ||
| [state, JSON.stringify(context), orderId, expectedVersion] | ||
| ); | ||
| if (res.rowCount === 0) { | ||
| throw new Error(`Optimistic concurrency failure: workflow ${orderId}`); | ||
| } | ||
| } else { | ||
| await pool.query( | ||
| `INSERT INTO purchase_workflows (order_id, user_id, state, context, version) | ||
| VALUES ($1, $2, $3, $4, 1) | ||
| ON CONFLICT (order_id) DO UPDATE SET | ||
| state = EXCLUDED.state, | ||
| context = EXCLUDED.context, | ||
| version = purchase_workflows.version + 1, | ||
| updated_at = CURRENT_TIMESTAMP`, | ||
| [orderId, userId, state, JSON.stringify(context)] | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Write the new DB version back into context.
Line 89 reuses the live record.context, but this function never updates _dbVersion after a successful write. Fresh workflows therefore stay on the unguarded UPSERT path forever, and recovered workflows will false-conflict on their second persisted transition because they still send the old version. Return the new version from SQL and assign it back to context._dbVersion.
Suggested fix
export async function persistWorkflowState(
orderId: string,
state: string,
context: Record<string, any>
): Promise<void> {
const pool = getPool();
const userId = context.userId;
const expectedVersion = context._dbVersion as number | undefined;
+ let res;
if (expectedVersion !== undefined) {
- const res = await pool.query(
+ res = await pool.query(
`UPDATE purchase_workflows
SET state = $1, context = $2, version = version + 1, updated_at = CURRENT_TIMESTAMP
- WHERE order_id = $3 AND version = $4`,
+ WHERE order_id = $3 AND version = $4
+ RETURNING version`,
[state, JSON.stringify(context), orderId, expectedVersion]
);
if (res.rowCount === 0) {
throw new Error(`Optimistic concurrency failure: workflow ${orderId}`);
}
} else {
- await pool.query(
+ res = await pool.query(
`INSERT INTO purchase_workflows (order_id, user_id, state, context, version)
VALUES ($1, $2, $3, $4, 1)
ON CONFLICT (order_id) DO UPDATE SET
state = EXCLUDED.state,
context = EXCLUDED.context,
version = purchase_workflows.version + 1,
- updated_at = CURRENT_TIMESTAMP`,
+ updated_at = CURRENT_TIMESTAMP
+ RETURNING version`,
[orderId, userId, state, JSON.stringify(context)]
);
}
+
+ context._dbVersion = res.rows[0].version;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** Saves current machine state and context. */ | |
| export async function persistWorkflowState( | |
| orderId: string, | |
| state: string, | |
| context: Record<string, any> | |
| ): Promise<void> { | |
| const pool = getPool(); | |
| const userId = context.userId; | |
| const expectedVersion = context._dbVersion as number | undefined; | |
| if (expectedVersion !== undefined) { | |
| const res = await pool.query( | |
| `UPDATE purchase_workflows | |
| SET state = $1, context = $2, version = version + 1, updated_at = CURRENT_TIMESTAMP | |
| WHERE order_id = $3 AND version = $4`, | |
| [state, JSON.stringify(context), orderId, expectedVersion] | |
| ); | |
| if (res.rowCount === 0) { | |
| throw new Error(`Optimistic concurrency failure: workflow ${orderId}`); | |
| } | |
| } else { | |
| await pool.query( | |
| `INSERT INTO purchase_workflows (order_id, user_id, state, context, version) | |
| VALUES ($1, $2, $3, $4, 1) | |
| ON CONFLICT (order_id) DO UPDATE SET | |
| state = EXCLUDED.state, | |
| context = EXCLUDED.context, | |
| version = purchase_workflows.version + 1, | |
| updated_at = CURRENT_TIMESTAMP`, | |
| [orderId, userId, state, JSON.stringify(context)] | |
| ); | |
| } | |
| /** Saves current machine state and context. */ | |
| export async function persistWorkflowState( | |
| orderId: string, | |
| state: string, | |
| context: Record<string, any> | |
| ): Promise<void> { | |
| const pool = getPool(); | |
| const userId = context.userId; | |
| const expectedVersion = context._dbVersion as number | undefined; | |
| let res; | |
| if (expectedVersion !== undefined) { | |
| res = await pool.query( | |
| `UPDATE purchase_workflows | |
| SET state = $1, context = $2, version = version + 1, updated_at = CURRENT_TIMESTAMP | |
| WHERE order_id = $3 AND version = $4 | |
| RETURNING version`, | |
| [state, JSON.stringify(context), orderId, expectedVersion] | |
| ); | |
| if (res.rowCount === 0) { | |
| throw new Error(`Optimistic concurrency failure: workflow ${orderId}`); | |
| } | |
| } else { | |
| res = await pool.query( | |
| `INSERT INTO purchase_workflows (order_id, user_id, state, context, version) | |
| VALUES ($1, $2, $3, $4, 1) | |
| ON CONFLICT (order_id) DO UPDATE SET | |
| state = EXCLUDED.state, | |
| context = EXCLUDED.context, | |
| version = purchase_workflows.version + 1, | |
| updated_at = CURRENT_TIMESTAMP | |
| RETURNING version`, | |
| [orderId, userId, state, JSON.stringify(context)] | |
| ); | |
| } | |
| context._dbVersion = res.rows[0].version; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/orchestrator/src/index.ts` around lines 33 - 64,
persistWorkflowState currently writes to purchase_workflows but never updates
context._dbVersion after a successful save, so callers keep using stale version
state. Update the persistWorkflowState flow to return the new version from the
INSERT/UPDATE SQL in both branches, then assign that version back onto
context._dbVersion before returning; use the existing persistWorkflowState and
expectedVersion logic to keep the optimistic concurrency path consistent.
Comment on lines
+252
to
+258
| ledgerEntry.status = status; | ||
| if (status === "CONFIRMED") { | ||
| ledgerEntry.confirmedAt = new Date(); | ||
| ledgerEntry.errorDetails = null; | ||
| } else { | ||
| ledgerEntry.errorDetails = error || "Transaction failed"; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
FAILED transitions should null out confirmedAt.
If a record is ever corrected from CONFIRMED to FAILED, this leaves both states set at once. The row should not retain a confirmation timestamp after entering the failure state.
Suggested fix
if (status === "CONFIRMED") {
ledgerEntry.confirmedAt = new Date();
ledgerEntry.errorDetails = null;
} else {
+ ledgerEntry.confirmedAt = null;
ledgerEntry.errorDetails = error || "Transaction failed";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ledgerEntry.status = status; | |
| if (status === "CONFIRMED") { | |
| ledgerEntry.confirmedAt = new Date(); | |
| ledgerEntry.errorDetails = null; | |
| } else { | |
| ledgerEntry.errorDetails = error || "Transaction failed"; | |
| } | |
| ledgerEntry.status = status; | |
| if (status === "CONFIRMED") { | |
| ledgerEntry.confirmedAt = new Date(); | |
| ledgerEntry.errorDetails = null; | |
| } else { | |
| ledgerEntry.confirmedAt = null; | |
| ledgerEntry.errorDetails = error || "Transaction failed"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/payments/events/index.ts` around lines 252 - 258, The status
transition logic in the ledger update path leaves confirmedAt populated when a
record moves from CONFIRMED to FAILED. Update the conditional handling in the
ledgerEntry status update block so that when the new status is not CONFIRMED,
confirmedAt is explicitly cleared along with setting errorDetails, keeping the
ledgerEntry state consistent.
Comment on lines
+11
to
+13
| pool: { | ||
| min: Number(process.env.DATABASE_POOL_MIN ?? 2), | ||
| max: Number(process.env.DATABASE_POOL_MAX ?? 10), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the pool env vars before constructing Sequelize.
Number(...) will happily turn a typo like "ten" into NaN, and this also accepts DATABASE_POOL_MIN > DATABASE_POOL_MAX. That pushes a simple config error into harder-to-diagnose pool failures at startup/runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/payments/src/db.ts` around lines 11 - 13, The Sequelize pool
config in db.ts should validate DATABASE_POOL_MIN and DATABASE_POOL_MAX before
constructing the pool settings instead of blindly using Number(...). Add
explicit parsing/validation so non-numeric values are rejected and ensure the
resolved min is not greater than max, then fail fast with a clear config error
before creating the Sequelize instance.
Comment on lines
+1
to
+8
| CREATE TABLE purchase_workflows ( | ||
| order_id VARCHAR(255) PRIMARY KEY, | ||
| user_id UUID NOT NULL, | ||
| state VARCHAR(100) NOT NULL, | ||
| context JSONB NOT NULL, | ||
| version INTEGER NOT NULL DEFAULT 1, | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Constrain state to the workflow-state contract.
Line 4 currently accepts any string, but Line 107 in apps/backend/orchestrator/src/index.ts later casts this column straight to PurchaseState, and Line 100 hard-codes terminal-state names. One typo or out-of-band update can persist a value the recovery path cannot safely restore. Add a CHECK/enum that matches the orchestrator's valid states.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@database/migrations/005_purchase_workflows.sql` around lines 1 - 8, The
purchase_workflows table currently allows any string in state, but the
orchestrator code in apps/backend/orchestrator/src/index.ts treats it as
PurchaseState and relies on a fixed set of valid terminal states. Update the
migration for purchase_workflows to constrain state with a CHECK constraint or
enum that matches the workflow-state contract used by PurchaseState and the
terminal-state logic, so only valid states can be persisted or restored.
Comment on lines
+34
to
+42
| try { | ||
| await client.query(migrationSql); | ||
| console.log(`[delego] db:migrate — migration ${file} applied successfully.`); | ||
| } catch (err) { | ||
| if (err.message && err.message.includes("already exists")) { | ||
| console.log(`[delego] db:migrate — warning: relation or type in ${file} already exists, skipping...`); | ||
| } else { | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Don't treat one duplicate-object error as success for the whole migration file.
client.query(migrationSql) executes the entire .sql file as a batch. If a later statement fails with "already exists", earlier statements in that same file may already have been committed, and this catch then suppresses the rest forever. database/migrations/002_gateway_auth_limits.sql already has that shape: users_email_key can fail after earlier ALTER TABLE statements, which means later DDL in that file may never run on rerun.
💡 Safer direction
+ await client.query(`
+ CREATE TABLE IF NOT EXISTS schema_migrations (
+ name TEXT PRIMARY KEY,
+ applied_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
+ )
+ `);
+
for (const file of files) {
+ const applied = await client.query(
+ "SELECT 1 FROM schema_migrations WHERE name = $1",
+ [file]
+ );
+ if (applied.rowCount > 0) continue;
+
console.log(`[delego] db:migrate — running migration: ${file}...`);
const migrationPath = path.join(migrationsDir, file);
const migrationSql = fs.readFileSync(migrationPath, "utf8");
+ await client.query("BEGIN");
try {
await client.query(migrationSql);
+ await client.query(
+ "INSERT INTO schema_migrations(name) VALUES ($1)",
+ [file]
+ );
+ await client.query("COMMIT");
console.log(`[delego] db:migrate — migration ${file} applied successfully.`);
} catch (err) {
- if (err.message && err.message.includes("already exists")) {
- console.log(`[delego] db:migrate — warning: relation or type in ${file} already exists, skipping...`);
- } else {
- throw err;
- }
+ await client.query("ROLLBACK");
+ throw err;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| await client.query(migrationSql); | |
| console.log(`[delego] db:migrate — migration ${file} applied successfully.`); | |
| } catch (err) { | |
| if (err.message && err.message.includes("already exists")) { | |
| console.log(`[delego] db:migrate — warning: relation or type in ${file} already exists, skipping...`); | |
| } else { | |
| throw err; | |
| } | |
| await client.query("BEGIN"); | |
| try { | |
| await client.query(migrationSql); | |
| await client.query( | |
| "INSERT INTO schema_migrations(name) VALUES ($1)", | |
| [file] | |
| ); | |
| await client.query("COMMIT"); | |
| console.log(`[delego] db:migrate — migration ${file} applied successfully.`); | |
| } catch (err) { | |
| await client.query("ROLLBACK"); | |
| throw err; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/setup/migrate.js` around lines 34 - 42, The migration retry handling
in migrate.js is too broad because the catch around client.query(migrationSql)
treats any "already exists" error as a successful file-level skip, which can
hide partially applied SQL batches. Update the migration flow so
duplicate-object errors are handled per statement or otherwise detected before
executing the batch, and do not mark the whole migration file as applied when
only part of it ran. Use the existing migrate script logic and the
client.query/migrationSql execution path to locate the fix, and ensure files
like 002_gateway_auth_limits.sql can be safely rerun without suppressing later
DDL.
Comment on lines
+45
to
+74
| it("should log transaction submission with PENDING status", async () => { | ||
| const hash = "0000000000000000000000000000000000000000000000000000000000000001"; | ||
| const method = "create_escrow"; | ||
| const orderId = "order_123"; | ||
| const contractId = "contract_abc"; | ||
|
|
||
| const entry = await logSubmission(hash, method, orderId, contractId); | ||
| assert.equal(entry.hash, hash); | ||
| assert.equal(entry.method, method); | ||
| assert.equal(entry.status, "PENDING"); | ||
| assert.equal(entry.orderId, orderId); | ||
| assert.equal(entry.contractId, contractId); | ||
| }); | ||
|
|
||
| it("should update ledger status to CONFIRMED", async () => { | ||
| const hash = "0000000000000000000000000000000000000000000000000000000000000001"; | ||
|
|
||
| const entry = await updateLedgerStatus(hash, "CONFIRMED"); | ||
| assert.equal(entry.status, "CONFIRMED"); | ||
| assert.ok(entry.confirmedAt); | ||
| assert.equal(entry.errorDetails, null); | ||
| }); | ||
|
|
||
| it("should update ledger status to FAILED with error details", async () => { | ||
| const hash = "0000000000000000000000000000000000000000000000000000000000000001"; | ||
|
|
||
| const entry = await updateLedgerStatus(hash, "FAILED", "Simulation failed"); | ||
| assert.equal(entry.status, "FAILED"); | ||
| assert.equal(entry.errorDetails, "Simulation failed"); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make these tests independent.
The CONFIRMED and FAILED cases depend on the first test having already created the row, so the suite becomes order-dependent and they stop validating updateLedgerStatus() in isolation. Per-test setup would also make it easy to cover the retry-to-PENDING path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/src/payments-ledger.test.js` around lines 45 - 74, The payments
ledger tests are order-dependent because `updateLedgerStatus()` is exercised
against state created by the previous `logSubmission()` test. Add per-test
setup/teardown in `payments-ledger.test.js` so each `it(...)` creates its own
submission record before calling `updateLedgerStatus`, and keep the
`logSubmission`/`updateLedgerStatus` assertions isolated. Use the existing
helpers directly in each test so `CONFIRMED`, `FAILED`, and any
retry-to-`PENDING` cases validate independently of test execution order.
Contributor
|
Fix the conflicts |
Contributor
|
Please fix the conflicts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #52
Summary
Type of change
Test plan
pnpm typecheckpnpm testChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation