Conversation
🦋 Changeset detectedLatest commit: 211a115 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a debt repayment notification system using BullMQ queues. It adds a maturity queue for periodically checking user debts across 24h and 1h windows, processing accounts in batches, selecting between market and previewer implementations, and triggering push notifications via Redis-based deduplication. Changes
Sequence DiagramsequenceDiagram
participant Scheduler as Scheduler
participant Queue as BullMQ Queue
participant Worker as Maturity Worker
participant DB as Database
participant Contract as Contract (Market/Previewer)
participant Redis as Redis
participant Sentry as Sentry
participant Push as Push Notifications
Scheduler->>Queue: scheduleMaturityChecks() creates<br/>CHECK_DEBTS jobs (24h, 1h)
Queue->>Worker: Job available
Worker->>DB: Read accounts in batch (250)
Worker->>Contract: Query debt status<br/>(implementation-dependent)
Contract->>Worker: Debt results per user
Worker->>Redis: Check notification<br/>idempotency key
alt Debt exists & not notified
Worker->>Redis: Write idempotency key
Worker->>Push: Send push notification
end
Worker->>Sentry: Log metrics & breadcrumbs<br/>(contract calls, errors, results)
alt Window is "1h"
Worker->>Queue: Schedule next maturity checks<br/>(increment maturity)
end
Worker->>Queue: Mark job complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Summary of ChangesHello @aguxez, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical new feature to proactively notify users about their upcoming debt maturities. It establishes a robust server-side queueing system using BullMQ to periodically check user debt positions on the blockchain. Depending on configuration, it leverages either direct market contract interactions or a previewer contract to identify at-risk users. Timely push notifications are then dispatched via OneSignal, with Redis ensuring that users receive alerts only once per window, aiming to help users manage their debts and avoid potential liquidations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
❌ 13 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Prevent Tests Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
6550d21 to
45be58c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
45be58c to
06f8efd
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
06f8efd to
ef1ce32
Compare
aguxez
force-pushed
the
feature/webhook-queue
branch
from
ef1ce32 to
26e6a32
Compare
aguxez
force-pushed
the
feature/webhook-queue
branch
from
26e6a32 to
2105108
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
2105108 to
b229c65
Compare
aguxez
force-pushed
the
feature/webhook-queue
branch
from
b229c65 to
e267d55
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
e267d55 to
fed4d42
Compare
aguxez
force-pushed
the
feature/webhook-queue
branch
from
fed4d42 to
1ed3cb9
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| const results = await Promise.allSettled([ | ||
| closeSentry(), | ||
| closeSegment(), | ||
| database.$client.end(), | ||
| closeMaturityQueue(), | ||
| closeRedis(), | ||
| ]); |
There was a problem hiding this comment.
Bug: A race condition exists between closeMaturityQueue() and closeRedis() during shutdown. The shared Redis connection might be closed before the BullMQ queue has finished its cleanup.
Severity: HIGH
Suggested Fix
Ensure the BullMQ queue is closed before its underlying Redis connection. Modify the shutdown sequence to await closeMaturityQueue() before calling await closeRedis(), removing them from the concurrent Promise.allSettled block and executing them sequentially.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: server/index.ts#L327-L333
Potential issue: During graceful shutdown, `closeMaturityQueue()` and `closeRedis()` are
executed concurrently via `Promise.allSettled`. However, the BullMQ queue and worker
managed by `closeMaturityQueue()` depend on the Redis connection that `closeRedis()`
terminates. If `closeRedis()` completes first, `closeMaturityQueue()` will fail when it
attempts to use the closed connection. This will cause an `AggregateError` to be thrown,
leading to the process exiting with a non-zero status code (1) instead of a clean
shutdown (0).
co-authored-by: danilo neves cruz <cruzdanilo@gmail.com>
^ Conflicts: ^ server/test/hooks/manteca.test.ts
| const markets = await Promise.all( | ||
| chunk.map(({ account }) => | ||
| publicClient.readContract({ | ||
| address: previewerAddress, | ||
| abi: previewerAbi, | ||
| functionName: "exactly", | ||
| args: [parse(Address, account)], | ||
| }), | ||
| ), | ||
| ); | ||
| totalContractCalls += chunk.length; |
There was a problem hiding this comment.
Bug: The use of Promise.all for batch RPC calls will cause the entire batch to fail if a single call rejects, potentially blocking notifications for other users.
Severity: MEDIUM
Suggested Fix
Replace Promise.all with Promise.allSettled. This will allow the process to continue for successful RPC calls even if some fail. You can then iterate over the results to handle fulfilled and rejected promises individually, ensuring that a single failure does not halt the entire batch.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: server/utils/maturity.ts#L62-L72
Potential issue: In `server/utils/maturity.ts`, `Promise.all` is used to execute
multiple `readContract` calls in parallel for a chunk of up to 50 accounts. If any
single RPC call fails (e.g., due to network issues or an invalid contract state),
`Promise.all` will reject immediately. This causes the entire job to fail and be
retried. If one account's RPC call consistently fails, it will block notifications for
all other accounts in its chunk after the retry attempts are exhausted, leading to
missed notifications.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 211a115f22
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| describe("t()", () => { | ||
| it("returns en and es translations with no options", () => { | ||
| const result = t("Card purchase"); | ||
| expect(result).toStrictEqual({ en: "Card purchase", es: "Compra con tarjeta" }); // cspell:ignore Compra tarjeta |
There was a problem hiding this comment.
This test file still asserts the old i18n contract (only en/es and keys like "...Paid with USDC"), but server/i18n/index.ts now returns en/es/pt and card-purchase copy is keyed via pluralized "...Paid in {{count}} installments" variants. Because server runs vitest run across test/**/*.test.ts, these strict assertions fail even when runtime behavior is correct, turning the test target red.
Useful? React with 👍 / 👎.
| describe("f()", () => { | ||
| describe("number input", () => { | ||
| it("preserves sub-micro values", () => { | ||
| expect(f(0.000_000_9)).toStrictEqual({ en: "0.0000009", es: "0,0000009" }); |
There was a problem hiding this comment.
🟡 All test assertions in test/i18n.test.ts missing pt key cause toStrictEqual failures
Every toStrictEqual assertion in server/test/i18n.test.ts checks against objects with only { en, es } keys, but both f() (server/i18n/index.ts:42-46) and t() (server/i18n/index.ts:28-33) always return { en, es, pt }. Since toStrictEqual requires exact deep equality (including identical key sets), every assertion fails because the actual result contains an extra pt property. For example, f(0.000_000_9) returns { en: "0.0000009", es: "0,0000009", pt: "0,0000009" } but the test expects { en: "0.0000009", es: "0,0000009" }. This affects all 12 toStrictEqual calls in the file. The sibling test file server/test/utils/i18n.test.ts correctly includes pt in all assertions.
Prompt for agents
Every toStrictEqual assertion in server/test/i18n.test.ts is missing the pt key. The f() and t() functions in server/i18n/index.ts always return { en, es, pt } but the test assertions only have { en, es }. Every single assertion in this file needs to be updated to include the pt value. For f() tests, pt formatting matches es (both use comma as decimal separator). For t() tests, the pt translations need to be looked up from server/i18n/pt.json. See server/test/utils/i18n.test.ts for the correct pattern that includes all three languages.
Was this helpful? React with 👍 or 👎 to provide feedback.
| it("interpolates plain string values into both languages", () => { | ||
| const result = t("{{localAmount}} at {{merchantName}}. Paid with USDC", { | ||
| localAmount: "$1,234.56", | ||
| merchantName: "Store", | ||
| }); | ||
| expect(result).toStrictEqual({ | ||
| en: "$1,234.56 at Store. Paid with USDC", | ||
| es: "$1,234.56 en Store. Pagado con USDC", // cspell:ignore Pagado | ||
| }); |
There was a problem hiding this comment.
🟡 t() tests in test/i18n.test.ts use non-existent translation keys producing wrong Spanish assertions
The t() tests at lines 49-80 use translation keys like "{{localAmount}} at {{merchantName}}. Paid with USDC" and "{{localAmount}} at {{merchantName}}. Paid with credit" that do not exist in any translation file (server/i18n/en.json, server/i18n/es.json, server/i18n/pt.json). When a key is missing, i18next falls back to returning the key string itself (with interpolation applied) for all languages. So the es output for these tests would be the same as en (e.g., "$1,234.56 at Store. Paid with USDC"), not the expected Spanish text "$1,234.56 en Store. Pagado con USDC". The assertions are fundamentally wrong — they expect translation behavior for keys that have no translations defined.
Prompt for agents
The t() tests in server/test/i18n.test.ts at lines 43-80 use translation keys that don't exist in any translation file (en.json, es.json, pt.json). Keys like '{{localAmount}} at {{merchantName}}. Paid with USDC' are not present in any JSON file. When i18next can't find a key, it falls back to the key string itself for all languages — so the es and pt outputs would be identical to en. The test assertions that expect different Spanish text (e.g. 'en Store. Pagado con USDC' instead of 'at Store. Paid with USDC') are incorrect. Either add these keys to the translation files, or update the test to use keys that already exist (like the ones tested in server/test/utils/i18n.test.ts which uses real keys from the JSON files).
Was this helpful? React with 👍 or 👎 to provide feedback.
| expect(sendPushNotification).toHaveBeenCalledWith({ | ||
| userId: bobAccount, | ||
| headings: { en: "Withdraw completed", es: "Retiro completado" }, // cspell:ignore Retiro completado | ||
| contents: { en: "3 USDC sent to alice.eth", es: "3 USDC enviados a alice.eth" }, // cspell:ignore enviados | ||
| }); | ||
| expect(sendPushNotification).toHaveBeenCalledWith({ | ||
| userId: bobAccount, | ||
| headings: { en: "Withdraw completed", es: "Retiro completado" }, | ||
| contents: { en: "4 USDC sent to alice.eth", es: "4 USDC enviados a alice.eth" }, | ||
| }); |
There was a problem hiding this comment.
🟡 Block test uses hardcoded { en, es } objects missing pt, causing toHaveBeenCalledWith failure
The test "sends withdraw notification after proposal execution" at server/test/hooks/block.test.ts:217-226 asserts headings and contents using hardcoded { en: "...", es: "..." } objects that are missing the pt key. The production code calls t() (server/i18n/index.ts:27-33) which always returns { en, es, pt }. Vitest's toHaveBeenCalledWith uses deep equality, so { en: "Withdraw completed", es: "Retiro completado" } does not match { en: "Withdraw completed", es: "Retiro completado", pt: "Saque concluído" }. Other tests in the same file (e.g., lines 156-173) correctly use the t() helper to produce expected values with all three language keys.
| expect(sendPushNotification).toHaveBeenCalledWith({ | |
| userId: bobAccount, | |
| headings: { en: "Withdraw completed", es: "Retiro completado" }, // cspell:ignore Retiro completado | |
| contents: { en: "3 USDC sent to alice.eth", es: "3 USDC enviados a alice.eth" }, // cspell:ignore enviados | |
| }); | |
| expect(sendPushNotification).toHaveBeenCalledWith({ | |
| userId: bobAccount, | |
| headings: { en: "Withdraw completed", es: "Retiro completado" }, | |
| contents: { en: "4 USDC sent to alice.eth", es: "4 USDC enviados a alice.eth" }, | |
| }); | |
| expect(sendPushNotification).toHaveBeenCalledWith({ | |
| userId: bobAccount, | |
| headings: t("Withdraw completed"), | |
| contents: t("{{amount}} {{symbol}} sent to {{recipient}}", { | |
| amount: f("3"), | |
| symbol: "USDC", | |
| recipient: "alice.eth", | |
| }), | |
| }); | |
| expect(sendPushNotification).toHaveBeenCalledWith({ | |
| userId: bobAccount, | |
| headings: t("Withdraw completed"), | |
| contents: t("{{amount}} {{symbol}} sent to {{recipient}}", { | |
| amount: f("4"), | |
| symbol: "USDC", | |
| recipient: "alice.eth", | |
| }), | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const results = await Promise.allSettled([ | ||
| closeSentry(), | ||
| closeSegment(), | ||
| database.$client.end(), | ||
| closeMaturityQueue(), | ||
| closeRedis(), | ||
| ]); |
There was a problem hiding this comment.
🚩 Shutdown race between closeRedis() and closeMaturityQueue()
In server/index.ts:327-333, closeMaturityQueue() and closeRedis() run concurrently via Promise.allSettled. BullMQ clones the provided Redis connection internally, so its own operations are safe. However, the maturity worker's processor (server/utils/maturity.ts:85-95) uses the original redis connection directly for redis.get(key) and redis.set(key, ...). If closeRedis() completes first, a job in progress could fail its dedup/notification logic. Since removeOnFail: true is set (server/utils/maturity.ts:24), a failed job would be permanently removed. On the next startup, scheduleMaturityChecks() reschedules future maturities, so this edge case is unlikely to cause persistent issues, but it could cause a notification to be missed during a shutdown that happens to coincide with job processing.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const markets = await Promise.all( | ||
| chunk.map(({ account }) => | ||
| publicClient.readContract({ | ||
| address: previewerAddress, | ||
| abi: previewerAbi, | ||
| functionName: "exactly", | ||
| args: [parse(Address, account)], | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🚩 Maturity worker RPC failure in one account blocks all accounts in the chunk
In server/utils/maturity.ts:62-71, the readContract calls for all accounts in a chunk are wrapped in Promise.all. If any single RPC call fails, the entire Promise.all rejects, skipping notification processing for all accounts in that chunk (and subsequent chunks). The job will be retried (3 attempts with exponential backoff per server/utils/maturity.ts:21-22), which handles transient failures. However, if one specific account consistently causes RPC errors (e.g., a contract that reverts on exactly() call), it would permanently block notifications for all accounts processed in the same chunk. Using Promise.allSettled instead of Promise.all for the RPC calls would allow unaffected accounts to still receive notifications.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
closes #337
Summary by CodeRabbit
Release Notes