🦺 server: filter notifications to known tokens only#974
Conversation
391e906 to
b5ee87a
Compare
🦋 Changeset detectedLatest commit: 8e731f9 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 |
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds Redis-cached LiFi token lookups to activity hook notification logic: notifications for incoming transfers are sent only if the asset is in marketsByAsset or known via per-chain LiFi token cache (fetch/cache on miss). Tests and Sentry error capturing updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Activity as Activity Hook
participant Redis as Redis Cache
participant LiFi as LiFi API
participant Sentry as Sentry
participant Notif as Notification Service
Activity->>Redis: SISMEMBER & SCARD (lifi:tokens:chainId)
alt Cache Hit
Redis-->>Activity: Token known? / set size
else Cache Miss
Activity->>LiFi: GET /v1/tokens?chains=chainId (5s timeout)
alt Success
LiFi-->>Activity: Token list
Activity->>Redis: DEL, SADD parsed addresses, EXPIRE 3600
Redis-->>Activity: OK
else Failure
LiFi-->>Activity: Error
Activity->>Sentry: captureException(error, { level: "error" })
Activity-->>Activity: assume token known (fail-open)
end
end
Activity-->>Activity: determine knownToken (marketsByAsset || isKnownToken)
alt Token Known
Activity->>Notif: sendPushNotification(...)
else Token Unknown
Activity-->>Notif: suppress notification
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #974 +/- ##
==========================================
+ Coverage 72.02% 73.37% +1.34%
==========================================
Files 229 229
Lines 8358 9059 +701
Branches 2692 3003 +311
==========================================
+ Hits 6020 6647 +627
- Misses 2106 2155 +49
- Partials 232 257 +25
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ecdf4f2 to
4d6172b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/hooks/activity.ts (1)
143-143:⚠️ Potential issue | 🟠 MajorBlocking webhook response on a per-transfer LiFi fetch.
await isKnownToken(chain.id, underlying)runs serially inside thefor (const ... of transfers)loop beforec.json({})returns. On cache miss, this adds up to 5s (plus Redis RTTs) per unknown-token transfer, serialized across transfers in the payload. Alchemy may retry slow webhooks, which can amplify downstream work (poking, autoCredit, duplicate sends).Consider resolving the known-token check off the response path — e.g. kick off the LiFi/Redis checks in parallel and gate only the
sendPushNotification(...)call on an already-resolved decision, so the handler acks quickly regardless of LiFi latency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3560abdd-a0de-493c-8582-db8f20162cb6
📒 Files selected for processing (4)
.changeset/sharp-sails-guess.mdcspell.jsonserver/hooks/activity.tsserver/test/hooks/activity.test.ts
4d6172b to
53dadc1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53dadc1439
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
53dadc1 to
7d31ae1
Compare
7d31ae1 to
8e731f9
Compare
Summary by CodeRabbit
Bug Fixes
Tests
Chores