fix(screener): fix the cache #3412
Conversation
WalkthroughThe changes introduce a new constant, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3412 +/- ##
===================================================
+ Coverage 3.10368% 26.04982% +22.94614%
===================================================
Files 44 254 +210
Lines 2932 15455 +12523
Branches 0 82 +82
===================================================
+ Hits 91 4026 +3935
- Misses 2838 11060 +8222
- Partials 3 369 +366
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
contrib/screener-api/chainalysis/chainalysisapi.go (2)
44-44: Consider making TTL configurableWhile 24 hours is a reasonable default TTL for caching risk assessments, different deployments might require different cache durations based on their risk policies and compliance requirements.
Consider making this configurable through environment variables or configuration files, similar to how
maxCacheSizeGBis planned to be configurable (as noted in the TODO comment).
139-140: Consider implementing negative cachingCurrently, non-risky addresses are simply deleted from the cache. This could lead to repeated API calls for frequently checked safe addresses. Consider implementing negative caching (caching the "safe" status) to reduce API calls.
Example implementation:
-c.registrationCache.Delete(address) +// Store false to indicate non-risky status +c.registrationCache.Set(address, false) // Update the cache check in ScreenAddress method: -if _, ok := c.registrationCache.Get(address); ok { +if val, ok := c.registrationCache.Get(address); ok { + // Return true only if the address was cached as risky + return val == struct{}{}, nil +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
contrib/screener-api/chainalysis/chainalysisapi.go(3 hunks)
🔇 Additional comments (1)
contrib/screener-api/chainalysis/chainalysisapi.go (1)
57-60: LGTM: Cache configuration is well-structured
The cache configuration is comprehensive with:
- LRU eviction policy for efficient memory usage
- Memory limits to prevent OOM issues
- Default TTL for automatic cleanup
Let's verify the cache configuration impact:
✅ Verification successful
Cache configuration is correctly implemented and isolated
The cache configuration is properly implemented with:
- Single instance of cache initialization in the codebase
- Appropriate constants defined for memory limits (3GB) and TTL (24 hours)
- Consistent usage throughout the file for address screening and registration
- No conflicting cache configurations found in other parts of the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for memory-related configurations and potential issues
# Look for other cache configurations that might conflict
rg -A 2 'NewCache\(\)'
# Look for memory-related configurations
rg 'WithMaxMemoryUsage|maxCacheSizeGB'
Length of output: 542
Script:
#!/bin/bash
# Check for any other cache configurations or memory settings in the file
cat contrib/screener-api/chainalysis/chainalysisapi.go
Length of output: 4811
Deploying sanguine-fe with
|
| Latest commit: |
076ff95
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://104d148d.sanguine-fe.pages.dev |
| Branch Preview URL: | https://fix-cache-screener.sanguine-fe.pages.dev |
|
This PR is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days. |
Description
A clear and concise description of the features you're adding in this pull request.
Additional context
Add any other context about the problem you're solving.
Metadata
Summary by CodeRabbit
New Features
Improvements