Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Summary by CodeRabbit
WalkthroughAdds a Canton fees adapter that fetches cached daily burn timeseries from an external API, converts burns to USD using an average amulet price, and returns dailyFees/dailyRevenue/dailyHoldersRevenue. Also adds Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ❌ 6❌ Failed checks (6 warnings)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
✨ Simplify code
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 |
|
The canton.ts adapter exports: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fees/canton.ts`:
- Line 34: The fetch function currently declares unused placeholder parameters
(_a: any, _b: any, options: FetchOptions); either remove the unused parameters
and change the signature to fetch(options: FetchOptions) if the adapter can use
the Version 2 pattern, or if the V1 adapter signature must be preserved, add a
concise inline comment above or next to the fetch declaration explaining that _a
and _b are required placeholders for the V1 adapter signature and intentionally
unused; update the function signature or comment in the fetch declaration
accordingly (refer to the fetch function and its parameters _a, _b, options:
FetchOptions).
- Line 48: The return currently reuses the same Balances object for dailyRevenue
and dailyHoldersRevenue which causes shared-reference bugs; create a distinct
Balances instance for dailyHoldersRevenue instead of reusing dailyRevenue (e.g.,
construct/clone a new object with the same numeric fields), and return {
dailyFees, dailyRevenue, dailyHoldersRevenue: <new cloned Balances> } so
downstream mutations to one do not affect the other.
- Around line 67-71: The breakdownMethodology currently documents only Fees but
dailyRevenue and dailyHoldersRevenue also use LABELS.TokenBurn; update the
breakdownMethodology object to add entries for Revenue and HoldersRevenue
(mirroring the Fees entry) so every label used in .add() calls is
documented—specifically add breakdownMethodology.Revenue and
breakdownMethodology.HoldersRevenue with a TokenBurn description matching the
Fees one and ensure LABELS.TokenBurn is referenced in those new entries.
- Around line 15-28: The module-level cached promise (cachedData) in getDataMap
has no invalidation so stale/erroneous API responses persist; modify the cache
to include a TTL by storing alongside the Promise a timestamp/expiry and check
it in getDataMap before returning cachedData, and when expired re-fetch via
fetchURL(API_URL) to replace cachedData; ensure you reference and update the
same cachedData variable (and any new expiry field) and keep the existing
mapping logic that turns res.data into Record<string, DailyData>.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eee71bf8-7193-4a78-a8e1-b26f62bff594
📒 Files selected for processing (2)
fees/canton.tshelpers/chains.ts
| let cachedData: Promise<Record<string, DailyData>> | null = null; | ||
|
|
||
| function getDataMap(): Promise<Record<string, DailyData>> { | ||
| if (!cachedData) { | ||
| cachedData = fetchURL(API_URL).then((res: { data: DailyData[] }) => { | ||
| const map: Record<string, DailyData> = {}; | ||
| res.data.forEach((entry) => { | ||
| map[entry.date] = entry; | ||
| }); | ||
| return map; | ||
| }); | ||
| } | ||
| return cachedData; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Module-level cache lacks invalidation mechanism.
The cached promise persists indefinitely across all adapter invocations. If the API returns stale or erroneous data, subsequent calls will continue using the bad cache until the process restarts. Consider adding a TTL or allowing the cache to refresh periodically, especially since historical data may be updated upstream.
♻️ Optional: Add TTL-based cache invalidation
-let cachedData: Promise<Record<string, DailyData>> | null = null;
+let cachedData: Promise<Record<string, DailyData>> | null = null;
+let cacheTimestamp = 0;
+const CACHE_TTL_MS = 60 * 60 * 1000; // 1 hour
function getDataMap(): Promise<Record<string, DailyData>> {
- if (!cachedData) {
+ const now = Date.now();
+ if (!cachedData || (now - cacheTimestamp) > CACHE_TTL_MS) {
+ cacheTimestamp = now;
cachedData = fetchURL(API_URL).then((res: { data: DailyData[] }) => {
const map: Record<string, DailyData> = {};
res.data.forEach((entry) => {
map[entry.date] = entry;
});
return map;
});
}
return cachedData;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fees/canton.ts` around lines 15 - 28, The module-level cached promise
(cachedData) in getDataMap has no invalidation so stale/erroneous API responses
persist; modify the cache to include a TTL by storing alongside the Promise a
timestamp/expiry and check it in getDataMap before returning cachedData, and
when expired re-fetch via fetchURL(API_URL) to replace cachedData; ensure you
reference and update the same cachedData variable (and any new expiry field) and
keep the existing mapping logic that turns res.data into Record<string,
DailyData>.
| TokenBurn: "Token Burn", | ||
| }; | ||
|
|
||
| export const fetch = async (_a: any, _b: any, options: FetchOptions) => { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify or remove the unused function parameters.
The signature (_a: any, _b: any, options: FetchOptions) uses placeholder parameters that are not utilized. While this may be required for the Version 1 adapter signature, consider adding a brief comment explaining why these parameters exist, or verify if Version 2 adapter pattern (which uses just options: FetchOptions) would be more appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fees/canton.ts` at line 34, The fetch function currently declares unused
placeholder parameters (_a: any, _b: any, options: FetchOptions); either remove
the unused parameters and change the signature to fetch(options: FetchOptions)
if the adapter can use the Version 2 pattern, or if the V1 adapter signature
must be preserved, add a concise inline comment above or next to the fetch
declaration explaining that _a and _b are required placeholders for the V1
adapter signature and intentionally unused; update the function signature or
comment in the fetch declaration accordingly (refer to the fetch function and
its parameters _a, _b, options: FetchOptions).
|
The canton.ts adapter exports: |
2 participants
No description provided.