feat: issues 175 171 137 136#289
Conversation
Emit the stored order metadata hash at escrow creation so indexers can link contract state to off-chain order records (closes DelegoLabs#175).
Expose get_merchant_receipt with release eligibility for merchant dashboards and settlement checks (closes DelegoLabs#171).
Track delivery failures and remove expired or repeatedly failing push subscriptions via cleanupPushSubscriptions (closes DelegoLabs#137).
Return TemplateRenderResult and refuse to send emails with missing template variables or failed renders (closes DelegoLabs#136).
📝 WalkthroughWalkthroughThe PR adds structured email template rendering errors, tracked push-subscription cleanup and failure recording, escrow metadata events, and a merchant receipt getter. Tests, documentation, and ledger snapshots cover notification behavior and escrow lifecycle states. ChangesNotification delivery resilience
Escrow metadata and merchant receipts
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)Notification deliverysequenceDiagram
participant ApprovalDispatcher
participant Redis
participant PushSubscription
participant Cleanup
ApprovalDispatcher->>Redis: read tracked subscriptions
ApprovalDispatcher->>PushSubscription: send notification
PushSubscription-->>ApprovalDispatcher: delivery failure
ApprovalDispatcher->>Redis: store incremented failure metadata
Cleanup->>Redis: remove stale or repeatedly failing records
Escrow metadata and receiptssequenceDiagram
participant DepositCaller
participant EscrowContract
participant Indexer
participant Merchant
DepositCaller->>EscrowContract: deposit with order metadata
EscrowContract-->>Indexer: EscrowMetadataEvent
Merchant->>EscrowContract: get_merchant_receipt(escrow_id)
EscrowContract-->>Merchant: MerchantEscrowReceipt
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/backend/notifications/src/permissionEventListener.ts (1)
751-760: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip malformed push subscriptions instead of dropping the whole set
parseTrackedPushSubscriptionthrows on bad JSON, and the outertry/catchturns that into[], so one corrupt Redis member drops every other valid subscription for this user. Parse each member defensively and keep the rest.🤖 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/permissionEventListener.ts` around lines 751 - 760, Update getUserPushSubscriptions so each Redis member is parsed defensively, skipping malformed entries while retaining valid subscriptions. Move error isolation into the members.map processing around parseTrackedPushSubscription, and ensure one parsing failure does not cause the outer try/catch to return an empty set for all members.apps/backend/notifications/src/dispatcher.ts (1)
53-65: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle malformed Redis members in
removePushSubscription
parseTrackedPushSubscriptionalready accepts legacy barePushSubscriptionJSON, but a single invalid member can still throw and stop the loop. Catch per-item parse errors so one corrupt record doesn’t blocksavePushSubscriptionfrom adding the new entry.🤖 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/dispatcher.ts` around lines 53 - 65, The removePushSubscription loop currently aborts when parseTrackedPushSubscription encounters a malformed Redis member. Wrap each member’s parsing and endpoint comparison in per-item error handling so invalid records are skipped while valid matching subscriptions are still removed and the loop continues, allowing savePushSubscription to add new entries.
🧹 Nitpick comments (4)
contracts/escrow/src/lib.rs (1)
1142-1169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueField
escrow_idstores the 32‑byteorder_id, not the numericescrow_idargument.
get_merchant_receiptis queried by theu64escrow_id, but the returnedMerchantEscrowReceipt.escrow_idis set fromrecord.order_id(aBytesN<32>). This mirrorsReleaseEligibility, so it may be intentional, but the same name meaning two different identifiers is easy to misread on the consumer side. Consider renaming the struct field toorder_idfor clarity, or documenting the mapping at the call boundary.This applies equally to
MerchantEscrowReceipt(Line 319) andEscrowMetadataEvent(Line 79).🤖 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 `@contracts/escrow/src/lib.rs` around lines 1142 - 1169, Clarify the identifier naming across MerchantEscrowReceipt and EscrowMetadataEvent: rename the receipt/event field currently named escrow_id to order_id where it stores record.order_id, and update get_merchant_receipt and all consumers or constructors accordingly. Preserve the numeric escrow_id lookup and any separate fields that represent the u64 key.contracts/escrow/src/integration_tests.rs (1)
755-769: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage:
Funded+ timed-out →release_eligible = false.Per the upstream
release_block_reasonlogic, aFundedescrow becomes release-ineligible onceenv.ledger().sequence() >= record.timeout_ledger, independent of status transitions. None of the new tests advance the ledger sequence, so only the "not yet timed out" branch ofFundedis exercised — the timeout branch of the release-eligibility logic (a core part of issue#171) remains untested.♻️ Suggested additional test
/// Success path: funded escrow past timeout is not release-eligible. #[test] fn test_get_merchant_receipt_funded_timeout_not_eligible() { let t = TestEnv::setup(); let client = EscrowContractClient::new(&t.env, &t.escrow_contract_id); let eid = deposit_escrow(&t, 1000, 100); t.env.ledger().with_mut(|li| li.sequence_number = 100); let receipt = client.get_merchant_receipt(&eid); assert_eq!(receipt.status, EscrowStatus::Funded); assert!(!receipt.release_eligible); }🤖 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 `@contracts/escrow/src/integration_tests.rs` around lines 755 - 769, Add a timeout-branch integration test alongside test_get_merchant_receipt_funded_state: after deposit_escrow creates a Funded escrow, advance the test ledger sequence to the escrow timeout boundary, call get_merchant_receipt, and assert the status remains EscrowStatus::Funded while release_eligible is false.apps/backend/notifications/templates/index.test.ts (1)
100-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage overall; one failure branch is untested.
The "unsubstituted placeholders remain after replace" branch in
renderTemplateContent(triggered whendatadoesn't cover every{{key}}, or via areplaceAllspecial-pattern reinsertion) has no direct test, nor does therenderNamedTemplateread/render exception branch.🤖 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/templates/index.test.ts` around lines 100 - 155, Add tests covering the unsubstituted-placeholder failure branch in renderTemplateContent, including a replacement pattern that leaves {{key}} tokens intact if applicable, and assert the returned error and absent html. Also add a renderNamedTemplate test that exercises a template file read/render exception and verifies the expected error result, using the existing helper APIs and result contract.apps/backend/notifications/push/index.ts (1)
173-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor inconsistency:
catchbranch drops entries from bothretainedandremoved.Malformed-shape entries (lines 179-183) are counted as
failedand pushed intoremoved, but entries caught via thecatchblock (191) are only counted infailed— they vanish from both output arrays despite being counted inscanned. For consistency (and to avoid silently dropped subscriptions if a caller rewrites Redis state fromretained/removed), consider pushing toremovedhere too. Low priority since the preceding validation likely makes this branch unreachable in practice.🤖 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/push/index.ts` around lines 173 - 192, Update the catch branch in the subscriptions loop to push the caught `tracked` entry into `removed` alongside incrementing `failed`, matching the malformed-entry handling and preventing it from being omitted from both output arrays.
🤖 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/push/index.ts`:
- Around line 42-50: Validate the environment-derived values in
getMaxPushFailures and getPushStaleMs so non-numeric or otherwise invalid values
cannot produce NaN and disable cleanup. Fall back to the existing defaults (or
fail explicitly) while preserving valid overrides, and ensure
shouldRemovePushSubscription continues evaluating both removal rules with usable
numeric thresholds.
In `@apps/backend/notifications/src/dispatcher.ts`:
- Around line 190-199: Update the sendPushNotification failure handler so
recordPushDeliveryFailure(tracked) is added to Redis with redis.sadd before
removing raw with redis.srem. Preserve the existing failure-record update and
cleanup behavior, ensuring an error during the add cannot cause the original
subscription to be deleted.
- Around line 71-99: Update cleanupUserPushSubscriptions to safely handle
malformed Redis members without aborting the user cleanup: isolate
parseTrackedPushSubscription failures and treat invalid records as discarded.
Replace the separate srem-all and sadd operations with one atomic MULTI/Lua
update that removes only discarded serialized members while preserving retained
subscriptions, including when the update fails partway.
- Around line 150-157: Update the Redis subscription processing in
dispatchTransactionApproval to isolate parseTrackedPushSubscription failures per
member. Skip malformed entries before building or iterating trackedList, while
continuing to process valid subscriptions so one corrupt Redis record cannot
abort dispatch.
In `@apps/backend/notifications/templates/index.ts`:
- Around line 95-121: Update renderTemplateContent so each data value is
HTML-escaped before replacing its corresponding placeholder, preventing
user-controlled markup injection. Use the replacement callback form of
replaceAll rather than a string replacement, ensuring dollar-sign sequences in
values are inserted literally; preserve the existing missing-variable and
unsubstituted-placeholder checks.
---
Outside diff comments:
In `@apps/backend/notifications/src/dispatcher.ts`:
- Around line 53-65: The removePushSubscription loop currently aborts when
parseTrackedPushSubscription encounters a malformed Redis member. Wrap each
member’s parsing and endpoint comparison in per-item error handling so invalid
records are skipped while valid matching subscriptions are still removed and the
loop continues, allowing savePushSubscription to add new entries.
In `@apps/backend/notifications/src/permissionEventListener.ts`:
- Around line 751-760: Update getUserPushSubscriptions so each Redis member is
parsed defensively, skipping malformed entries while retaining valid
subscriptions. Move error isolation into the members.map processing around
parseTrackedPushSubscription, and ensure one parsing failure does not cause the
outer try/catch to return an empty set for all members.
---
Nitpick comments:
In `@apps/backend/notifications/push/index.ts`:
- Around line 173-192: Update the catch branch in the subscriptions loop to push
the caught `tracked` entry into `removed` alongside incrementing `failed`,
matching the malformed-entry handling and preventing it from being omitted from
both output arrays.
In `@apps/backend/notifications/templates/index.test.ts`:
- Around line 100-155: Add tests covering the unsubstituted-placeholder failure
branch in renderTemplateContent, including a replacement pattern that leaves
{{key}} tokens intact if applicable, and assert the returned error and absent
html. Also add a renderNamedTemplate test that exercises a template file
read/render exception and verifies the expected error result, using the existing
helper APIs and result contract.
In `@contracts/escrow/src/integration_tests.rs`:
- Around line 755-769: Add a timeout-branch integration test alongside
test_get_merchant_receipt_funded_state: after deposit_escrow creates a Funded
escrow, advance the test ledger sequence to the escrow timeout boundary, call
get_merchant_receipt, and assert the status remains EscrowStatus::Funded while
release_eligible is false.
In `@contracts/escrow/src/lib.rs`:
- Around line 1142-1169: Clarify the identifier naming across
MerchantEscrowReceipt and EscrowMetadataEvent: rename the receipt/event field
currently named escrow_id to order_id where it stores record.order_id, and
update get_merchant_receipt and all consumers or constructors accordingly.
Preserve the numeric escrow_id lookup and any separate fields that represent the
u64 key.
🪄 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: d407039b-aa3e-432e-aff6-62f4b2a3e3df
📒 Files selected for processing (21)
apps/backend/notifications/README.mdapps/backend/notifications/email/errorClassifier.tsapps/backend/notifications/email/index.test.tsapps/backend/notifications/email/index.tsapps/backend/notifications/push/index.test.tsapps/backend/notifications/push/index.tsapps/backend/notifications/src/dispatcher.tsapps/backend/notifications/src/permissionEventListener.tsapps/backend/notifications/templates/index.test.tsapps/backend/notifications/templates/index.tscontracts/escrow/src/integration_tests.rscontracts/escrow/src/lib.rscontracts/escrow/src/test.rscontracts/escrow/test_snapshots/integration_tests/test_get_merchant_receipt_disputed_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_merchant_receipt_funded_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_merchant_receipt_not_found.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_merchant_receipt_refunded_state.1.jsoncontracts/escrow/test_snapshots/integration_tests/test_get_merchant_receipt_released_state.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_with_metadata_emits_metadata_event.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_with_partial_metadata_does_not_emit_metadata_event.1.jsoncontracts/escrow/test_snapshots/test/test/test_deposit_without_metadata_does_not_emit_metadata_event.1.json
2 participants
Closes #175
Closes #171
Closes #137
Closes #136
Summary by CodeRabbit
New Features
Bug Fixes
Tests