Feature/escrow event listener#264
Open
Danitello123 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a Soroban escrow contract event polling listener to the notifications service that decodes on-chain events and dispatches idempotent email and push notifications. Simultaneously adds a PostgreSQL-backed transaction ledger to the payments service, including a Sequelize model, SQL migration, automatic migration runner, and ledger write functions. ChangesNotifications: Escrow Event Listener
Payments: Soroban Transaction Ledger
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 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: 7
🤖 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/src/escrowListener.ts`:
- Around line 193-224: The idempotency key is being marked too early in
escrowListener’s send flow, so a failed email or push delivery still gets
recorded as dispatched. Update the escrowListener logic around
checkAndMarkDispatched, sendEmail, and the push send path so the key is only
marked after a successful delivery, or is cleared/released inside the failure
catch when delivery fails. Apply the same fix consistently for both the email
and push branches, including the later push block referenced in the diff.
- Around line 47-67: The cursor logic in escrowListener is too coarse:
startLedger/lastProcessedKey only tracks the last ledger, so getEvents
pagination and per-event failures can skip same-ledger overflow permanently.
Update EscrowListener to use an event-granular checkpoint (such as the RPC
paging token or a ledger-plus-event-index cursor) instead of only
latestLedger/last_ledger, and advance that checkpoint only after each event is
successfully processed. Make the replay path resume from the last successfully
handled event in the processing loop, not from the next ledger.
- Around line 47-54: The cold-start ledger calculation in escrowListener’s
checkpoint logic can produce a non-positive startLedger when Redis has no saved
value and latestLedger is below 100. Update the start ledger initialization in
the escrowListener flow so it is clamped to a minimum valid ledger (for example,
never below 1) before calling getEvents, while preserving the existing Redis
checkpoint behavior for the lastProcessedKey/latestLedger path.
In `@apps/backend/payments/events/index.ts`:
- Around line 252-258: The status update logic in the payment event handler
leaves stale confirmation data behind when a row transitions to FAILED. Update
the status branch in the relevant ledger entry update flow so that the same path
that sets status and errorDetails also clears confirmedAt for any non-CONFIRMED
outcome, and keep the CONFIRMED branch setting confirmedAt and clearing
errorDetails as it does now. Use the existing ledgerEntry status assignment
block in the payments events index handler to make this change.
- Around line 213-219: The retry path in the existing ledger row update leaves
stale terminal data behind, so `ledgerEntry` can be `PENDING` while still
carrying `confirmedAt` or `errorDetails` from a prior `FAILED`/`CONFIRMED`
state. In the `if (!created)` block inside the transaction submission flow,
reset the terminal fields when reusing the row by clearing `confirmedAt` and
`errorDetails` along with setting `status` back to `PENDING`, then continue
updating the existing `ledgerEntry` before saving it.
In `@database/migrations/004_soroban_transaction_ledger.sql`:
- Around line 21-24: The inline rollback block in the migration file is being
executed as part of the normal apply path, so remove the down-migration
statements from this file. Update the 004_soroban_transaction_ledger migration
content so it only contains the forward schema changes for
soroban_transaction_ledger and its indexes, and keep any rollback logic out of
the SQL passed to migrate.js.
In `@tests/unit/src/payments-ledger.test.js`:
- Line 1: The payment ledger tests are order-dependent because the CONFIRMED and
FAILED cases rely on state seeded by the first case via mockLedgerEntries for
the same hash. Update the test setup in the payments-ledger suite so each case
seeds its own ledger row independently, likely in the individual test bodies or
a per-test setup helper used by the relevant describe block. Keep the tests
isolated by ensuring each case creates the needed mockLedgerEntries entry before
asserting on CONFIRMED or FAILED behavior.
🪄 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: 5bf133a0-efc2-4670-93a8-7e9383fef14a
📒 Files selected for processing (12)
apps/backend/notifications/README.mdapps/backend/notifications/src/escrowListener.test.tsapps/backend/notifications/src/escrowListener.tsapps/backend/notifications/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.sqlscripts/setup/migrate.jstests/unit/src/payments-ledger.test.js
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 cold-start ledger floor.
When Redis has no checkpoint and latestLedger < 100, this computes startLedger <= 0. The first getEvents call then polls an invalid ledger range on fresh/test networks.
Proposed 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
cold-start ledger calculation in escrowListener’s checkpoint logic can produce a
non-positive startLedger when Redis has no saved value and latestLedger is below
100. Update the start ledger initialization in the escrowListener flow so it is
clamped to a minimum valid ledger (for example, never below 1) before calling
getEvents, while preserving the existing Redis checkpoint behavior for the
lastProcessedKey/latestLedger path.
Comment on lines
+47
to
+67
| 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; | ||
| } | ||
|
|
||
| if (startLedger <= latestLedger) { | ||
| 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
Ledger-only checkpointing can drop events permanently.
getEvents is capped at 1000, but the persisted cursor is just latestLedger. That skips any same-ledger overflow and any event that throws during processing, because the next poll resumes at lastLedger + 1 instead of replaying from the last successfully handled event. This listener needs an event-granular cursor (RPC paging token or ledger + event index) and should only advance it after successful handling.
Also applies to: 69-124
🤖 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 - 67, The
cursor logic in escrowListener is too coarse: startLedger/lastProcessedKey only
tracks the last ledger, so getEvents pagination and per-event failures can skip
same-ledger overflow permanently. Update EscrowListener to use an event-granular
checkpoint (such as the RPC paging token or a ledger-plus-event-index cursor)
instead of only latestLedger/last_ledger, and advance that checkpoint only after
each event is successfully processed. Make the replay path resume from the last
successfully handled event in the processing loop, not from the next ledger.
Comment on lines
+193
to
+224
| 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 }) | ||
| ) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Push | ||
| if (target.pushEnabled) { | ||
| const shouldSend = await checkAndMarkDispatched(redis, { | ||
| userId: target.userId, | ||
| channel: "push", | ||
| eventType: event.eventType, | ||
| eventId, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don't consume the idempotency key before a successful send.
Both channels call checkAndMarkDispatched before delivery, then catch and log send failures. A transient SendGrid/web-push failure or a bad stored subscription therefore becomes a permanent drop, because retries are now suppressed by Redis. Mark success after delivery, or clear the key on failure.
Also applies to: 226-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 - 224, The
idempotency key is being marked too early in escrowListener’s send flow, so a
failed email or push delivery still gets recorded as dispatched. Update the
escrowListener logic around checkAndMarkDispatched, sendEmail, and the push send
path so the key is only marked after a successful delivery, or is
cleared/released inside the failure catch when delivery fails. Apply the same
fix consistently for both the email and push branches, including the later push
block referenced in the diff.
Comment on lines
+213
to
+219
| if (!created) { | ||
| log.warn("Transaction submission ledger entry already exists, updating for retry", { hash }); | ||
| ledgerEntry.status = "PENDING"; | ||
| ledgerEntry.method = method; | ||
| if (orderId) ledgerEntry.orderId = orderId; | ||
| if (contractId) ledgerEntry.contractId = contractId; | ||
| await ledgerEntry.save(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset terminal fields when reusing an existing ledger row.
If this hash was previously FAILED or CONFIRMED, this retry path only flips status back to PENDING. The old confirmedAt and errorDetails remain, so one row can read as pending and already settled at the same time.
🛠️ Proposed fix
if (!created) {
log.warn("Transaction submission ledger entry already exists, updating for retry", { hash });
ledgerEntry.status = "PENDING";
ledgerEntry.method = method;
- if (orderId) ledgerEntry.orderId = orderId;
- if (contractId) ledgerEntry.contractId = contractId;
+ ledgerEntry.confirmedAt = null;
+ ledgerEntry.errorDetails = null;
+ ledgerEntry.submittedAt = new Date();
+ if (orderId !== undefined) ledgerEntry.orderId = orderId;
+ if (contractId !== undefined) ledgerEntry.contractId = contractId;
await ledgerEntry.save();
}📝 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
| if (!created) { | |
| log.warn("Transaction submission ledger entry already exists, updating for retry", { hash }); | |
| ledgerEntry.status = "PENDING"; | |
| ledgerEntry.method = method; | |
| if (orderId) ledgerEntry.orderId = orderId; | |
| if (contractId) ledgerEntry.contractId = contractId; | |
| await ledgerEntry.save(); | |
| if (!created) { | |
| log.warn("Transaction submission ledger entry already exists, updating for retry", { hash }); | |
| ledgerEntry.status = "PENDING"; | |
| ledgerEntry.method = method; | |
| ledgerEntry.confirmedAt = null; | |
| ledgerEntry.errorDetails = null; | |
| ledgerEntry.submittedAt = new Date(); | |
| if (orderId !== undefined) ledgerEntry.orderId = orderId; | |
| if (contractId !== undefined) ledgerEntry.contractId = contractId; | |
| await ledgerEntry.save(); |
🤖 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 213 - 219, The retry path
in the existing ledger row update leaves stale terminal data behind, so
`ledgerEntry` can be `PENDING` while still carrying `confirmedAt` or
`errorDetails` from a prior `FAILED`/`CONFIRMED` state. In the `if (!created)`
block inside the transaction submission flow, reset the terminal fields when
reusing the row by clearing `confirmedAt` and `errorDetails` along with setting
`status` back to `PENDING`, then continue updating the existing `ledgerEntry`
before saving it.
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
Clear confirmedAt when marking a row as failed.
A row that was previously confirmed keeps its old confirmation timestamp in the FAILED branch. That leaves the persisted state contradictory.
🛠️ Proposed 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
update logic in the payment event handler leaves stale confirmation data behind
when a row transitions to FAILED. Update the status branch in the relevant
ledger entry update flow so that the same path that sets status and errorDetails
also clears confirmedAt for any non-CONFIRMED outcome, and keep the CONFIRMED
branch setting confirmedAt and clearing errorDetails as it does now. Use the
existing ledgerEntry status assignment block in the payments events index
handler to make this change.
Comment on lines
+21
to
+24
| -- Down migration | ||
| DROP INDEX IF EXISTS idx_soroban_tx_ledger_order_id; | ||
| DROP INDEX IF EXISTS idx_soroban_tx_ledger_status; | ||
| DROP TABLE IF EXISTS soroban_transaction_ledger; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
Remove the inline rollback block from this migration.
scripts/setup/migrate.js executes the full file with one client.query(migrationSql) call, so these DROP ... statements run during normal migration application. That leaves soroban_transaction_ledger gone immediately after creation.
🛠️ Proposed fix
- -- Down migration
- DROP INDEX IF EXISTS idx_soroban_tx_ledger_order_id;
- DROP INDEX IF EXISTS idx_soroban_tx_ledger_status;
- DROP TABLE IF EXISTS soroban_transaction_ledger;📝 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
| -- Down migration | |
| DROP INDEX IF EXISTS idx_soroban_tx_ledger_order_id; | |
| DROP INDEX IF EXISTS idx_soroban_tx_ledger_status; | |
| DROP TABLE IF EXISTS soroban_transaction_ledger; |
🤖 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/004_soroban_transaction_ledger.sql` around lines 21 - 24,
The inline rollback block in the migration file is being executed as part of the
normal apply path, so remove the down-migration statements from this file.
Update the 004_soroban_transaction_ledger migration content so it only contains
the forward schema changes for soroban_transaction_ledger and its indexes, and
keep any rollback logic out of the SQL passed to migrate.js.
| @@ -0,0 +1,75 @@ | |||
| import { describe, it, before, after } from "node:test"; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make each test case seed its own ledger row.
The CONFIRMED and FAILED cases depend on the first test populating mockLedgerEntries for the same hash. That makes this suite order-dependent and brittle when cases are run or debugged in isolation.
🛠️ Proposed fix
-import { describe, it, before, after } from "node:test";
+import { describe, it, before, after, beforeEach } from "node:test";
@@
before(() => {
@@
});
+
+ beforeEach(() => {
+ mockLedgerEntries = {};
+ });
@@
it("should update ledger status to CONFIRMED", async () => {
const hash = "0000000000000000000000000000000000000000000000000000000000000001";
-
+ await logSubmission(hash, "create_escrow");
const entry = await updateLedgerStatus(hash, "CONFIRMED");
@@
it("should update ledger status to FAILED with error details", async () => {
const hash = "0000000000000000000000000000000000000000000000000000000000000001";
-
+ await logSubmission(hash, "create_escrow");
const entry = await updateLedgerStatus(hash, "FAILED", "Simulation failed");Also applies to: 45-74
🤖 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` at line 1, The payment ledger tests
are order-dependent because the CONFIRMED and FAILED cases rely on state seeded
by the first case via mockLedgerEntries for the same hash. Update the test setup
in the payments-ledger suite so each case seeds its own ledger row
independently, likely in the individual test bodies or a per-test setup helper
used by the relevant describe block. Keep the tests isolated by ensuring each
case creates the needed mockLedgerEntries entry before asserting on CONFIRMED or
FAILED behavior.
Contributor
|
Resolve 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 #56
Summary
Type of change
Test plan
pnpm typecheckpnpm testChecklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests