✨ server: add debt repayment notification#722
Draft
aguxez wants to merge 1 commit intofeature/webhook-queuefrom
Draft
✨ server: add debt repayment notification#722aguxez wants to merge 1 commit intofeature/webhook-queuefrom
aguxez wants to merge 1 commit intofeature/webhook-queuefrom
Conversation
🦋 Changeset detectedLatest commit: 9efc43d 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}
There was a problem hiding this comment.
Code Review
This pull request introduces a debt repayment notification system. However, a high-severity Denial of Service vulnerability was identified due to unbounded memory consumption when fetching users, along with a medium-severity logic error causing the notification system to stop rescheduling itself. Additionally, the review suggests improvements for scalability by optimizing database queries, enhancing type safety, and fixing a potential bug in the maturity calculation logic.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Fix all issues with AI agents
In `@server/queues/markets.ts`:
- Line 3: The array export DEBT_NOTIFICATION_MARKETS is missing a trailing
comma; update the declaration for DEBT_NOTIFICATION_MARKETS (the constant using
marketUSDCAddress and marketWETHAddress) to include a trailing comma after
marketWETHAddress inside the array literal so it follows the project's
trailing-comma style.
In `@server/queues/maturityQueue.ts`:
- Around line 66-74: The current cast on results to access .errors is fragile;
define a shared return type (e.g., DebtCheckResult with contractCalls, users
array, and optional errors?: Map<string, unknown>) and update both
checkDebtsPreviewerImplementation and checkDebtsMarketImplementation to return
that type so callers (like the code in maturityQueue.ts) can safely read
results.errors without casting; update the function signatures and return shapes
accordingly and remove the (results as ...) cast in the block handling
implementation === "previewer".
- Around line 15-17: The env var parsing for DEBT_NOTIFICATION_IMPLEMENTATION
currently casts any string to ImplementationType which hides invalid values;
update the initialization of implementation to read
process.env.DEBT_NOTIFICATION_IMPLEMENTATION as a raw string, validate it
against the allowed values ("market" | "previewer") (using the
ImplementationType union or a small set/array), and if it's invalid either throw
a clear error or log and fall back to a safe default; ensure you reference the
ImplementationType type and the implementation constant so the check occurs
before any code branches like implementation === "market" run.
- Around line 207-208: The call to publicClient.readContract is using a
redundant cast ``market as `0x${string}```; remove the unnecessary cast and pass
market directly (it already comes from DEBT_NOTIFICATION_MARKETS typed as
readonly [Address, Address]), and if TypeScript complains, adjust the
DEBT_NOTIFICATION_MARKETS/market declaration to be the correct Address type
rather than casting at the call site; locate the usage in maturityQueue.ts
inside the publicClient.readContract invocation and replace the casted argument
with market.
In `@server/test/queues/maturityQueue.test.ts`:
- Around line 318-341: The test "uses previewer implementation when configured"
is placed under the "market implementation" describe but validates the previewer
flow; move this it-block to the existing "previewer implementation" describe (or
to the top-level worker describe) so tests are grouped correctly—relocate the
entire it(...) that imports "../../queues/maturityQueue", uses
previewerProcessor, previewerAddress/previewerAbi, mockUsers and
mocks.readContract expectations into the "previewer implementation" describe
block and remove the original copy from the "market implementation" block.
- Line 350: Replace the hard-coded MATURITY_INTERVAL constant in
maturityQueue.test.ts with an import from the source module to avoid
duplication: remove the local declaration of MATURITY_INTERVAL and import the
exported MATURITY_INTERVAL from server/utils/fixedLibrary.ts (the symbol name is
MATURITY_INTERVAL) so tests stay in sync with the implementation; update any
references in the test file to use the imported MATURITY_INTERVAL.
- Around line 257-264: The mock for mocks.readContract is returning the same
encoded payload for every function call and should discriminate by the requested
function; update the mockImplementation for mocks.readContract to inspect the
first arg's functionName (like the existing pattern around the other mock) and
return the appropriate response for "accounts" vs "fixedBorrowPositions" (e.g.,
return [encoded, 0n, 0n] for the accounts call and the other tuple for
fixedBorrowPositions), preserving the callCount fallback behavior if needed so
the test mirrors the real call sequence and doesn't mask incorrect call
ordering.
In `@server/test/utils/fixedLibrary.test.ts`:
- Around line 62-66: The test "handles offset zero (base maturity)" is redundant
with the existing "returns true when base maturity matches"; remove the
redundant it(...) block or change it to cover a different scenario (e.g., test a
non-zero offset or the case where the maturity bit is unset) so it exercises a
unique code path in hasMaturity; locate the test by the it title "handles offset
zero (base maturity)" and update or delete it accordingly, ensuring the
remaining tests still assert hasMaturity(encoded, baseMaturity) behavior for
distinct cases.
In `@server/utils/fixedLibrary.ts`:
- Around line 1-2: The MATURITY_INTERVAL constant is duplicated; export
MATURITY_INTERVAL from fixedLibrary.ts so other modules reuse it as the single
source of truth: add an export for the existing MATURITY_INTERVAL symbol in
fixedLibrary.ts, remove the duplicate local definition in the other module (the
one that currently defines its own MATURITY_INTERVAL), and update that module to
import MATURITY_INTERVAL from fixedLibrary.ts; keep MASK_BASE_MATURITY
unchanged.
| const module_ = await import("../../queues/maturityQueue"); | ||
| const processor = module_.processor; | ||
|
|
||
| const MATURITY_INTERVAL = 2_419_200; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Import MATURITY_INTERVAL from the source module.
Same duplication issue as in fixedLibrary.test.ts. Import from server/utils/fixedLibrary.ts to stay in sync.
♻️ Proposed fix
+import { MATURITY_INTERVAL } from "../../utils/fixedLibrary";
+
// ... later in the test
- const MATURITY_INTERVAL = 2_419_200;
const jobMaturity = MATURITY_INTERVAL * 10;📝 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 MATURITY_INTERVAL = 2_419_200; | |
| import { MATURITY_INTERVAL } from "../../utils/fixedLibrary"; | |
| // ... rest of imports and test setup ... | |
| const jobMaturity = MATURITY_INTERVAL * 10; |
🤖 Prompt for AI Agents
In `@server/test/queues/maturityQueue.test.ts` at line 350, Replace the hard-coded
MATURITY_INTERVAL constant in maturityQueue.test.ts with an import from the
source module to avoid duplication: remove the local declaration of
MATURITY_INTERVAL and import the exported MATURITY_INTERVAL from
server/utils/fixedLibrary.ts (the symbol name is MATURITY_INTERVAL) so tests
stay in sync with the implementation; update any references in the test file to
use the imported MATURITY_INTERVAL.
Comment on lines
62
to
66
| it("handles offset zero (base maturity)", () => { | ||
| const baseMaturity = 1_000_000; | ||
| const encoded = BigInt(baseMaturity) | (1n << 32n); | ||
| expect(hasMaturity(encoded, baseMaturity)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant test case.
This test is functionally identical to "returns true when base maturity matches" (Lines 18-22). Both verify offset zero with bit 0 set. Consider removing or differentiating.
🤖 Prompt for AI Agents
In `@server/test/utils/fixedLibrary.test.ts` around lines 62 - 66, The test
"handles offset zero (base maturity)" is redundant with the existing "returns
true when base maturity matches"; remove the redundant it(...) block or change
it to cover a different scenario (e.g., test a non-zero offset or the case where
the maturity bit is unset) so it exercises a unique code path in hasMaturity;
locate the test by the it title "handles offset zero (base maturity)" and update
or delete it accordingly, ensuring the remaining tests still assert
hasMaturity(encoded, baseMaturity) behavior for distinct cases.
server/utils/fixedLibrary.ts
Outdated
Comment on lines
1
to
2
| const MATURITY_INTERVAL = 2_419_200; | ||
| const MASK_BASE_MATURITY = (1n << 32n) - 1n; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider exporting MATURITY_INTERVAL to avoid duplication.
This constant is also defined in server/queues/maturityQueue.ts (line 19). Exporting it from here would maintain a single source of truth.
Proposed fix
-const MATURITY_INTERVAL = 2_419_200;
+export const MATURITY_INTERVAL = 2_419_200;
const MASK_BASE_MATURITY = (1n << 32n) - 1n;📝 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 MATURITY_INTERVAL = 2_419_200; | |
| const MASK_BASE_MATURITY = (1n << 32n) - 1n; | |
| export const MATURITY_INTERVAL = 2_419_200; | |
| const MASK_BASE_MATURITY = (1n << 32n) - 1n; |
🤖 Prompt for AI Agents
In `@server/utils/fixedLibrary.ts` around lines 1 - 2, The MATURITY_INTERVAL
constant is duplicated; export MATURITY_INTERVAL from fixedLibrary.ts so other
modules reuse it as the single source of truth: add an export for the existing
MATURITY_INTERVAL symbol in fixedLibrary.ts, remove the duplicate local
definition in the other module (the one that currently defines its own
MATURITY_INTERVAL), and update that module to import MATURITY_INTERVAL from
fixedLibrary.ts; keep MASK_BASE_MATURITY unchanged.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
6550d21 to
45be58c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/queues/maturityQueue.ts`:
- Around line 308-324: The delay expressions for the two scheduled adds can be
negative if now is past the target time; update the delay calculation used in
getMaturityQueue().add (for MaturityJob.CHECK_DEBTS jobs with jobId
`check-debts-${nextMaturity}-24h` and `check-debts-${nextMaturity}-1h`) to clamp
to zero (e.g., compute the rawDelay as (nextMaturity - 24*3600 - now) * 1000 and
(nextMaturity - 3600 - now) * 1000 and then use Math.max(0, rawDelay) or
equivalent) so BullMQ never receives a negative delay.
- Line 31: Remove the duplicate MATURITY_INTERVAL constant from
server/queues/maturityQueue.ts and instead import the exported MATURITY_INTERVAL
from server/utils/fixedLibrary.ts (ensure fixedLibrary.ts exports
MATURITY_INTERVAL if not already). Update the top of maturityQueue.ts to import
{ MATURITY_INTERVAL } and delete the local const declaration so all code in
functions like the maturity processing logic uses the single shared constant.
In `@server/test/utils/fixedLibrary.test.ts`:
- Line 5: Replace the hard-coded MATURITY_INTERVAL constant in the test with an
import from the module that defines it: export MATURITY_INTERVAL from the source
module (fixedLibrary.ts) if not already exported, then in the test file remove
the local declaration and import { MATURITY_INTERVAL } from that module so the
test uses the canonical value from fixedLibrary rather than a duplicated
literal.
45be58c to
06f8efd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/queues/maturityQueue.ts`:
- Around line 310-312: The close() function currently awaits closing
maturityWorker and _maturityQueue but leaves their references intact, causing
initializeWorker() to no-op on subsequent calls; after awaiting the closures in
close(), reset the module-level references (maturityWorker and _maturityQueue)
to undefined (or null) so initializeWorker() can create new instances on
re-initialization.
In `@server/utils/redis.ts`:
- Line 5: The shared Redis client exported currently with "new
Redis(process.env.REDIS_URL, { maxRetriesPerRequest: null })" must not be used
on request-path handlers because null causes indefinite retries; create two
clients instead: keep a background client (e.g., backgroundRedis) with
maxRetriesPerRequest: null for queues/workers, and add a separate request-safe
client (e.g., redisClient or requestRedis) with a bounded retry strategy (set
maxRetriesPerRequest to a finite number) and a connectTimeout to be used by
request handlers referenced in authentication.ts, registration.ts and
hooks/block.ts; update exports so request handlers import the bounded client
while queue code uses the background client.
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
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@server/queues/maturityQueue.ts`:
- Around line 60-61: The processor function assumes job.data matches
CheckDebtsData but lacks runtime validation; add a valibot parse using the
existing checkDebtsSchema at the start of export const processor (after
verifying job.name === MaturityJob.CHECK_DEBTS) to validate and canonicalize
job.data, handle parse errors by logging and failing/acknowledging the job
cleanly (rather than letting downstream code throw), and then use the parsed
value for the rest of the function.
- Around line 84-93: The previewer-specific error handling inside the shared
processing loop (the "if (implementation === 'previewer')" block that inspects
results.errors for each account) should be removed from maturityQueue's
iteration so the loop treats all implementations uniformly; instead, modify the
previewer implementation that produces results (the code that returns
results.users and results.errors) to pre-filter out users with errors, skip
ContractFunctionExecutionError cases silently, and call captureException(error,
{ extra: { account } }) for other errors before returning the sanitized
results.users array so the main loop can assume no per-user previewer errors
remain.
- Line 228: The cast "const [fixedBorrows] = result.value as [bigint, bigint,
bigint]" is unsafe; instead validate result.value at runtime before
destructuring—use valibot to define a tuple schema (e.g., a 3-item bigint tuple)
and call parse/safeParse on result.value (from the Promise.allSettled result) or
at minimum assert Array.isArray(result.value) && result.value.length >= 3; apply
the same validation for the similar cast around the other occurrence (the
[bigint, bigint] case) so fixedBorrows and any other destructured values are
only used after successful validation.
- Around line 269-283: Remove the redundant isInitializing guard: delete the
isInitializing variable and all reads/writes to it inside initializeWorker and
close(); rely solely on the maturityWorker check in initializeWorker to prevent
duplicate initialization, and remove the isInitializing = false assignment in
the close() implementation; keep the new Worker(QUEUE_NAME, processor, {
connection: redis }) try/catch and captureException logic unchanged.
- Around line 176-184: AccountCall and PositionCall have identical shapes;
remove the duplicate by keeping a single unified type (e.g., AccountCall or a
clearer name like AccountMarketCall) and replace all references to PositionCall
with that unified type; update any imports/exports and usages in functions or
interfaces in maturityQueue.ts that reference PositionCall to use the new single
type so the duplicate declaration is removed and all callers compile against the
single definition.
- Around line 213-217: Rename the variable userDebtMap to remove the type from
its name (e.g., userDebt) and update every usage accordingly: change the
declaration "const userDebtMap = new Map<string, boolean>();", the population
loop "for (const { account } of chunk) { userDebtMap.set(account, false); }",
and all later reads/writes that reference userDebtMap (the places the map is
accessed later) to the new identifier so the code compiles and behavior is
unchanged.
- Around line 68-74: The pagination loop using CHUNK_SIZE with
database.select(...).from(credentials).limit(...).offset(offset) is missing a
deterministic ORDER BY; update the query to include a stable ordering (e.g.,
.orderBy(credentials.id) or .orderBy('id') or another unique/stable column) so
rows are consistently paged in the for loop, and optionally consider switching
to keyset pagination (filtering by > lastId) instead of OFFSET for large tables.
In `@server/utils/redis.ts`:
- Around line 5-7: Replace the default Redis client import in hooks/block.ts
(currently importing the default unbounded-retry client) with the named
requestRedis export so request-handling code uses bounded retries; update the
import statement to import { requestRedis } from the redis utility and replace
any usages of the default export (the anonymous Redis instance) in
hooks/block.ts with requestRedis (ensure any existing variable names align with
requestRedis).
server/utils/redis.ts
Outdated
Comment on lines
5
to
7
| export default new Redis(process.env.REDIS_URL, { maxRetriesPerRequest: null }); | ||
|
|
||
| export const requestRedis = new Redis(process.env.REDIS_URL, { maxRetriesPerRequest: 3 }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "from.*utils/redis" server --type ts -g '!**/test/**' -g '!**/mocks/**'Repository: exactly/exa
Length of output: 443
hooks/block.ts must use requestRedis instead of the default export.
The import on line 56 of hooks/block.ts still uses the default Redis client (with unbounded retries). Since block.ts runs on request paths, it should import requestRedis to enforce bounded retry behavior on requests. Queue files correctly use the default export; API auth files have already been migrated to requestRedis.
🤖 Prompt for AI Agents
In `@server/utils/redis.ts` around lines 5 - 7, Replace the default Redis client
import in hooks/block.ts (currently importing the default unbounded-retry
client) with the named requestRedis export so request-handling code uses bounded
retries; update the import statement to import { requestRedis } from the redis
utility and replace any usages of the default export (the anonymous Redis
instance) in hooks/block.ts with requestRedis (ensure any existing variable
names align with requestRedis).
2105108 to
b229c65
Compare
aguxez
force-pushed
the
feature/webhook-queue
branch
from
b229c65 to
e267d55
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@server/queues/maturityQueue.ts`:
- Around line 185-199: The loop builds accountPromises (Promise<unknown> from
publicClient.readContract) for every account×market which can create too many
concurrent RPC calls; modify the logic in maturityQueue.ts to batch these calls
into smaller sub-batches (e.g., slice accountCalls and accountPromises into
groups of N, or use a concurrency limiter like p-limit) and await each batch
before launching the next, applying the same batching for positionPromises as
well; locate the code around accountCalls, accountPromises,
publicClient.readContract, DEBT_NOTIFICATION_MARKETS and marketAbi and replace
the unbounded push+Promise.all pattern with batched/limited concurrency
execution to avoid RPC rate-limit failures.
- Around line 138-172: The function checkDebtsPreviewerImplementation currently
fires publicClient.readContract for every item in chunk via Promise.all causing
up to ~250 concurrent RPC calls; replace the unconstrained Promise.all with a
bounded-concurrency approach (e.g., use p-map or p-limit, or process chunk in
smaller batches) when invoking publicClient.readContract for previewerAddress
using previewerAbi/function "exactly", preserve the existing error handling for
ContractFunctionExecutionError and captureException, and ensure contractCalls
reflects the number of attempted calls (use chunk.length or actual attempts if
batching changes semantics).
- Around line 93-114: The concurrent notification logic collects promises in
notifications (from redis.set(...).then(...)) and uses await
Promise.all(notifications), which can cause unhandled rejections for remaining
in-flight promises; change this to await Promise.allSettled(notifications) and
iterate the settled results to log or report any failures (both rejected
redis.set and rejected sendPushNotification) so failures are surfaced instead of
becoming unhandled; update the handling around the notifications array,
redis.set(...).then(...), and sendPushNotification(...) to ensure errors are
captured and included in your logs/metrics.
In `@server/test/queues/maturityQueue.test.ts`:
- Around line 220-221: Remove the redundant local alias baseMaturity and inline
maturity directly where it's used: replace "const baseMaturity = maturity; const
encoded = BigInt(baseMaturity) | (1n << 32n);" with "const encoded =
BigInt(maturity) | (1n << 32n);" and make the same change at the second
occurrence (the other pair of lines around the same test), updating any
references to baseMaturity to use maturity instead.
Comment on lines
+93
to
+114
| const notifications: Promise<unknown>[] = []; | ||
| for (const { account, hasDebt } of results.users) { | ||
| if (hasDebt) { | ||
| notifications.push( | ||
| redis | ||
| .set(`notification:sent:${account}:${maturity}:${window}`, String(Date.now()), "EX", 86_400, "NX") | ||
| .then((r) => { | ||
| if (r === "OK") { | ||
| return sendPushNotification({ | ||
| userId: account, | ||
| headings: { en: "Debt Maturity Alert" }, | ||
| contents: { | ||
| en: `Your debt is due in ${window === "24h" ? "24 hours" : "1 hour"}. Repay now to avoid liquidation.`, | ||
| }, | ||
| }); | ||
| } | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| await Promise.all(notifications); |
There was a problem hiding this comment.
Promise.all can leave unhandled rejections from concurrent notification sends.
If one sendPushNotification rejects, Promise.all rejects immediately but the remaining in-flight promises continue. Any subsequent rejection becomes an unhandled promise rejection (potential crash with --unhandled-rejections=throw).
Use Promise.allSettled and report failures:
Proposed fix
- await Promise.all(notifications);
+ const settled = await Promise.allSettled(notifications);
+ for (const result of settled) {
+ if (result.status === "rejected") captureException(result.reason);
+ }🤖 Prompt for AI Agents
In `@server/queues/maturityQueue.ts` around lines 93 - 114, The concurrent
notification logic collects promises in notifications (from
redis.set(...).then(...)) and uses await Promise.all(notifications), which can
cause unhandled rejections for remaining in-flight promises; change this to
await Promise.allSettled(notifications) and iterate the settled results to log
or report any failures (both rejected redis.set and rejected
sendPushNotification) so failures are surfaced instead of becoming unhandled;
update the handling around the notifications array, redis.set(...).then(...),
and sendPushNotification(...) to ensure errors are captured and included in your
logs/metrics.
Comment on lines
+138
to
+172
| async function checkDebtsPreviewerImplementation( | ||
| chunk: { account: string }[], | ||
| maturity: number, | ||
| ): Promise<DebtCheckResult> { | ||
| const results = await Promise.all( | ||
| chunk.map(({ account }) => | ||
| publicClient | ||
| .readContract({ | ||
| address: previewerAddress, | ||
| abi: previewerAbi, | ||
| functionName: "exactly", | ||
| args: [account as `0x${string}`], | ||
| }) | ||
| .then((result) => ({ status: "success" as const, result, account })) | ||
| .catch((error: unknown) => ({ status: "failure" as const, error, account })), | ||
| ), | ||
| ); | ||
|
|
||
| const users: { account: string; hasDebt: boolean }[] = []; | ||
| for (const result of results) { | ||
| if (result.status === "failure") { | ||
| if (result.error instanceof ContractFunctionExecutionError) continue; | ||
| captureException(result.error, { extra: { account: result.account } }); | ||
| continue; | ||
| } | ||
| const hasDebt = result.result.some((marketAccount) => | ||
| marketAccount.fixedBorrowPositions.some( | ||
| (position) => Number(position.maturity) === maturity && position.position.principal > 0n, | ||
| ), | ||
| ); | ||
| users.push({ account: result.account, hasDebt }); | ||
| } | ||
|
|
||
| return { contractCalls: chunk.length, users }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Previewer implementation fires all RPC calls concurrently with no concurrency bound.
Same concern as the market implementation — Promise.all over the full chunk (up to 250 calls) with no backpressure. If this is acceptable given your RPC provider's limits, disregard.
🤖 Prompt for AI Agents
In `@server/queues/maturityQueue.ts` around lines 138 - 172, The function
checkDebtsPreviewerImplementation currently fires publicClient.readContract for
every item in chunk via Promise.all causing up to ~250 concurrent RPC calls;
replace the unconstrained Promise.all with a bounded-concurrency approach (e.g.,
use p-map or p-limit, or process chunk in smaller batches) when invoking
publicClient.readContract for previewerAddress using previewerAbi/function
"exactly", preserve the existing error handling for
ContractFunctionExecutionError and captureException, and ensure contractCalls
reflects the number of attempted calls (use chunk.length or actual attempts if
batching changes semantics).
Comment on lines
+185
to
+199
| const accountCalls: AccountCall[] = []; | ||
| const accountPromises: Promise<unknown>[] = []; | ||
| for (const { account } of chunk) { | ||
| for (const market of DEBT_NOTIFICATION_MARKETS) { | ||
| accountCalls.push({ account, market }); | ||
| accountPromises.push( | ||
| publicClient.readContract({ | ||
| address: market, | ||
| abi: marketAbi, | ||
| functionName: "accounts", | ||
| args: [account as `0x${string}`], | ||
| }), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
accountPromises collects promises typed as Promise<unknown> — consider batching to avoid unbounded concurrency.
For a chunk of 250 accounts × 2 markets = 500 concurrent readContract calls, plus potentially another round of positionPromises. If the RPC provider has rate limits, this could cause throttling or failures. Consider batching with a concurrency limiter or smaller sub-batches.
🤖 Prompt for AI Agents
In `@server/queues/maturityQueue.ts` around lines 185 - 199, The loop builds
accountPromises (Promise<unknown> from publicClient.readContract) for every
account×market which can create too many concurrent RPC calls; modify the logic
in maturityQueue.ts to batch these calls into smaller sub-batches (e.g., slice
accountCalls and accountPromises into groups of N, or use a concurrency limiter
like p-limit) and await each batch before launching the next, applying the same
batching for positionPromises as well; locate the code around accountCalls,
accountPromises, publicClient.readContract, DEBT_NOTIFICATION_MARKETS and
marketAbi and replace the unbounded push+Promise.all pattern with
batched/limited concurrency execution to avoid RPC rate-limit failures.
Comment on lines
+220
to
+221
| const baseMaturity = maturity; | ||
| const encoded = BigInt(baseMaturity) | (1n << 32n); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant baseMaturity alias.
baseMaturity is assigned from maturity and used once — inline it directly.
♻️ Proposed fix
- const baseMaturity = maturity;
- const encoded = BigInt(baseMaturity) | (1n << 32n);
+ const encoded = BigInt(maturity) | (1n << 32n);Apply in both occurrences (lines 220-221 and 274-275).
As per coding guidelines, "if a value or expression is used exactly once, keep it at the point of use. extract only for reuse or genuine complexity."
Also applies to: 274-275
🤖 Prompt for AI Agents
In `@server/test/queues/maturityQueue.test.ts` around lines 220 - 221, Remove the
redundant local alias baseMaturity and inline maturity directly where it's used:
replace "const baseMaturity = maturity; const encoded = BigInt(baseMaturity) |
(1n << 32n);" with "const encoded = BigInt(maturity) | (1n << 32n);" and make
the same change at the second occurrence (the other pair of lines around the
same test), updating any references to baseMaturity to use maturity instead.
aguxez
force-pushed
the
feature/webhook-queue
branch
from
e267d55 to
fed4d42
Compare
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
1 participant
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.
This is part 2 of 2 in a stack made with GitButler:
Summary by CodeRabbit
Release Notes