Refactor Chain assets#12222
Conversation
📝 WalkthroughWalkthroughThis PR introduces dual-layer persistence for chain assets: a SQL TVL cache table for daily rollups and DynamoDB for hourly snapshots. It adds a new storage module, integrates it into the asset and backfill pipelines with best-effort error handling, provides a flow fetch path from DynamoDB with legacy fallback, and supplies a migration script to backfill historical daily data from the existing source. ChangesChain Assets Dual-Write Storage and Migration
Sequence DiagramsequenceDiagram
participant ResData as Asset Response
participant TransformCell as transformCell()
participant DailyDB as DAILY_CHAIN_ASSETS
participant DDB as DynamoDB
participant FetchDaily as fetchDailyChainAssetsCharts()
participant FetchFlows as fetchFlowsFromDDB()
ResData->>TransformCell: parse chain, rawCell, day
TransformCell-->>TransformCell: validate JSON, extract data
TransformCell->>DailyDB: storeDailyChainAssets() bulk upsert
ResData->>DDB: storeHourlyChainAssets() write snapshot
DailyDB->>FetchDaily: query by timestamp ascending
FetchDaily-->>FetchDaily: reshape to per-chain arrays
DDB->>FetchFlows: query closest two snapshots
FetchFlows-->>FetchFlows: compute delta perc/raw
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
defi/l2/index.ts (1)
20-21: 💤 Low valueImprove error message specificity.
The generic error message doesn't indicate which operation failed (daily or hourly storage). Consider separate error messages or include operation context to aid debugging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defi/l2/index.ts` around lines 20 - 21, The catch block logging "storing daily/hourly chain assets failed" is too generic; update the error handling in the catch inside defi/l2/index.ts (the try/catch around the daily/hourly asset storage calls) to include which operation failed (daily vs hourly) and any identifying context (e.g., operation name/variable, chain id, timestamp) along with the error object; replace the single generic console.error with a more specific message that interpolates the operation context and the caught error to make debugging easier.defi/l2/cli/store24hFlow.ts (1)
13-13: 💤 Low valueComment may be misleading about the fallback source.
The comment mentions "falling back to coins2", but the
fetchFlowsfunction queries the postgreschainassetstable (see relevant context snippet at defi/l2/storeToDb.ts:242-289). Verify whether "coins2" is accurate or update the comment to reflect the actual fallback source.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defi/l2/cli/store24hFlow.ts` at line 13, The console.error message in store24hFlow.ts incorrectly says "falling back to coins2" even though fetchFlows queries the Postgres chainassets table; update the error string to accurately state the fallback source (e.g., "falling back to Postgres (chainassets)") and ensure any related logging around the fetchFlows call references the fetchFlows function and the chainassets table (see storeToDb.ts where fetchFlows reads chainassets) so the message is not misleading.defi/l2/cli/migrateDailyChainAssets.ts (1)
19-23: 💤 Low valueTypo in function name and clarify auth usage.
- Function name
iniSourceDbConnectionshould beinitSourceDbConnection.- The validation checks for 3 auth params but only
auth[0]is used. Consider adding a brief comment explaining what the expected format is (e.g., connection string at index 0).✏️ Suggested fix
-async function iniSourceDbConnection() { +async function initSourceDbConnection() { const auth = process.env.COINS2_AUTH?.split(",") ?? []; - if (!auth || auth.length != 3) throw new Error("there aren't 3 auth params"); + if (!auth || auth.length != 3) throw new Error("there aren't 3 auth params"); + // auth[0] is the connection string; auth[1] and auth[2] are reserved for other services return postgres(auth[0], { idle_timeout: 90 }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defi/l2/cli/migrateDailyChainAssets.ts` around lines 19 - 23, Rename the function iniSourceDbConnection to initSourceDbConnection and update all call sites to match; inside initSourceDbConnection keep the auth parsing but either use the other two auth elements or add a clear comment above the auth parsing explaining the expected COINS2_AUTH format (e.g., "COINS2_AUTH expected as 'connectionString,username,password' and only connectionString (auth[0]) is used here") and adjust the validation accordingly (if only auth[0] is required, validate for at least 1 element instead of 3; if 3 are required, wire auth[1]/auth[2] into the postgres call or related auth logic). Ensure the unique symbol initSourceDbConnection and the auth variable are updated consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@defi/l2/chainAssetsDb.ts`:
- Around line 116-118: When handling a chain/section present in end but missing
in start inside the flow calculation, don't set zeros; instead read the actual
end value and mark it as new growth. Update the branch that checks if
(!start.data[chain] || !(k in start.data[chain])) so that res[chain][k].raw =
String(end.data[chain][k]) (or the same numeric type used elsewhere) and
res[chain][k].perc = "100" (or the chosen representation for new-entry growth)
rather than { perc: "0", raw: "0" }; ensure you reference start.data, end.data,
res, chain and k when making the change.
In `@defi/l2/index.ts`:
- Around line 17-19: The current sequential awaits mean
storeHourlyChainAssets(res) is skipped if storeDailyChainAssets(res) throws;
change the logic so both independent writes are attempted regardless of the
other's failure—either run them in parallel and handle results with
Promise.allSettled([storeDailyChainAssets(res), storeHourlyChainAssets(res)])
and log/errors per result, or wrap each call in its own try-catch (keep calls to
storeDailyChainAssets and storeHourlyChainAssets) so each write logs its error
without preventing the other from running.
---
Nitpick comments:
In `@defi/l2/cli/migrateDailyChainAssets.ts`:
- Around line 19-23: Rename the function iniSourceDbConnection to
initSourceDbConnection and update all call sites to match; inside
initSourceDbConnection keep the auth parsing but either use the other two auth
elements or add a clear comment above the auth parsing explaining the expected
COINS2_AUTH format (e.g., "COINS2_AUTH expected as
'connectionString,username,password' and only connectionString (auth[0]) is used
here") and adjust the validation accordingly (if only auth[0] is required,
validate for at least 1 element instead of 3; if 3 are required, wire
auth[1]/auth[2] into the postgres call or related auth logic). Ensure the unique
symbol initSourceDbConnection and the auth variable are updated consistently.
In `@defi/l2/cli/store24hFlow.ts`:
- Line 13: The console.error message in store24hFlow.ts incorrectly says
"falling back to coins2" even though fetchFlows queries the Postgres chainassets
table; update the error string to accurately state the fallback source (e.g.,
"falling back to Postgres (chainassets)") and ensure any related logging around
the fetchFlows call references the fetchFlows function and the chainassets table
(see storeToDb.ts where fetchFlows reads chainassets) so the message is not
misleading.
In `@defi/l2/index.ts`:
- Around line 20-21: The catch block logging "storing daily/hourly chain assets
failed" is too generic; update the error handling in the catch inside
defi/l2/index.ts (the try/catch around the daily/hourly asset storage calls) to
include which operation failed (daily vs hourly) and any identifying context
(e.g., operation name/variable, chain id, timestamp) along with the error
object; replace the single generic console.error with a more specific message
that interpolates the operation context and the caught error to make debugging
easier.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 160bb715-8e23-4a11-9ad7-0664ba7b540b
📒 Files selected for processing (5)
defi/l2/chainAssetsDb.tsdefi/l2/cli/migrateDailyChainAssets.tsdefi/l2/cli/store24hFlow.tsdefi/l2/index.tsdefi/src/api2/db/tables.ts
| if (!start.data[chain] || !(k in start.data[chain])) { | ||
| res[chain][k] = { perc: "0", raw: "0" }; | ||
| return; |
There was a problem hiding this comment.
Incorrect handling of new chains/sections in flow calculation.
When a chain or section exists in the end snapshot but not in start (indicating it's new in this period), the current code returns { perc: "0", raw: "0" }. This is incorrect because:
rawshould be the actual value fromend.data[chain][k](the full value, since start is 0)percshould indicate growth, typically "100" or similar to show it's a new entry
Returning zeros makes it appear there's no change when in fact a new chain/section has appeared with a non-zero value.
🐛 Proposed fix
if (!start.data[chain] || !(k in start.data[chain])) {
- res[chain][k] = { perc: "0", raw: "0" };
+ const b = Number(end.data[chain][k]);
+ res[chain][k] = { perc: b === 0 ? "0" : "100", raw: b.toFixed() };
return;
}📝 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.
| if (!start.data[chain] || !(k in start.data[chain])) { | |
| res[chain][k] = { perc: "0", raw: "0" }; | |
| return; | |
| if (!start.data[chain] || !(k in start.data[chain])) { | |
| const b = Number(end.data[chain][k]); | |
| res[chain][k] = { perc: b === 0 ? "0" : "100", raw: b.toFixed() }; | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@defi/l2/chainAssetsDb.ts` around lines 116 - 118, When handling a
chain/section present in end but missing in start inside the flow calculation,
don't set zeros; instead read the actual end value and mark it as new growth.
Update the branch that checks if (!start.data[chain] || !(k in
start.data[chain])) so that res[chain][k].raw = String(end.data[chain][k]) (or
the same numeric type used elsewhere) and res[chain][k].perc = "100" (or the
chosen representation for new-entry growth) rather than { perc: "0", raw: "0" };
ensure you reference start.data, end.data, res, chain and k when making the
change.
| try { | ||
| await storeDailyChainAssets(res); | ||
| await storeHourlyChainAssets(res); |
There was a problem hiding this comment.
Sequential execution skips hourly write if daily write fails.
The sequential await pattern means that if storeDailyChainAssets throws (e.g., database connection error, constraint violation), storeHourlyChainAssets will never execute. Since these are independent writes to different storage layers (SQL for daily, DynamoDB for hourly), both should be attempted even if one fails to maximize coverage of the best-effort dual-write.
🔄 Proposed fix to ensure both writes are attempted
// dual-write to the new tables while readers are being moved over
try {
- await storeDailyChainAssets(res);
- await storeHourlyChainAssets(res);
+ await Promise.allSettled([
+ storeDailyChainAssets(res),
+ storeHourlyChainAssets(res),
+ ]);
} catch (e) {
console.error("storing daily/hourly chain assets failed", e);
}Alternatively, use separate try-catch blocks:
// dual-write to the new tables while readers are being moved over
- try {
- await storeDailyChainAssets(res);
- await storeHourlyChainAssets(res);
- } catch (e) {
- console.error("storing daily/hourly chain assets failed", e);
- }
+ try {
+ await storeDailyChainAssets(res);
+ } catch (e) {
+ console.error("storing daily chain assets failed", e);
+ }
+ try {
+ await storeHourlyChainAssets(res);
+ } catch (e) {
+ console.error("storing hourly chain assets failed", e);
+ }📝 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.
| try { | |
| await storeDailyChainAssets(res); | |
| await storeHourlyChainAssets(res); | |
| try { | |
| await Promise.allSettled([ | |
| storeDailyChainAssets(res), | |
| storeHourlyChainAssets(res), | |
| ]); | |
| } catch (e) { | |
| console.error("storing daily/hourly chain assets failed", e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@defi/l2/index.ts` around lines 17 - 19, The current sequential awaits mean
storeHourlyChainAssets(res) is skipped if storeDailyChainAssets(res) throws;
change the logic so both independent writes are attempted regardless of the
other's failure—either run them in parallel and handle results with
Promise.allSettled([storeDailyChainAssets(res), storeHourlyChainAssets(res)])
and log/errors per result, or wrap each call in its own try-catch (keep calls to
storeDailyChainAssets and storeHourlyChainAssets) so each write logs its error
without preventing the other from running.
|
Question on historical corrections: we fix past token prices/project TVL fairly often (almost daily for various projects), and today those get pushed into chain-assets history by re-running the backfill CLIs ( How would those corrections make it into the new Is the plan to rerun |
Added dual writes to the backfill CLIs |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@defi/l2/cli/backfillAll.ts`:
- Around line 29-34: The `storeDailyChainAssets(...)` call failures are being
logged but then swallowed, allowing timestamps to be marked as successful even
when the new daily table write fails, creating silent data divergence. In
`defi/l2/cli/backfillAll.ts` lines 29-34, after logging the error in the catch
block, rethrow the caught error so that it propagates up and the timestamp is
captured in the errors collection. Apply the identical fix to
`defi/l2/cli/backfillSingle.ts` lines 52-57 to ensure single-chain backfills
also fail visibly when the daily table write fails.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 752c1101-f4eb-46db-955e-523ee2c30b94
📒 Files selected for processing (2)
defi/l2/cli/backfillAll.tsdefi/l2/cli/backfillSingle.ts
| // dual-write to the new daily table | ||
| try { | ||
| await storeDailyChainAssets(res); | ||
| } catch (e) { | ||
| console.error("storing daily chain assets failed", e); | ||
| } |
There was a problem hiding this comment.
Do not swallow daily dual-write failures during backfill.
Both backfill CLIs currently log and continue when storeDailyChainAssets(...) fails, so a timestamp can be counted as successful even though only the legacy table was corrected. That creates silent divergence in historical data between old and new storage.
defi/l2/cli/backfillAll.ts#L29-L34: after logging, rethrow the error (or return a failure signal) so the timestamp is captured inerrorsand not counted as success.defi/l2/cli/backfillSingle.ts#L52-L57: apply the same behavior so single-chain backfills also fail visibly when the new daily table write fails.
Suggested patch pattern
try {
await storeDailyChainAssets(resOrWrite);
} catch (e) {
- console.error("storing daily chain assets failed", e);
+ console.error("storing daily chain assets failed", { timestamp, error: e });
+ throw e;
}📍 Affects 2 files
defi/l2/cli/backfillAll.ts#L29-L34(this comment)defi/l2/cli/backfillSingle.ts#L52-L57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@defi/l2/cli/backfillAll.ts` around lines 29 - 34, The
`storeDailyChainAssets(...)` call failures are being logged but then swallowed,
allowing timestamps to be marked as successful even when the new daily table
write fails, creating silent data divergence. In `defi/l2/cli/backfillAll.ts`
lines 29-34, after logging the error in the catch block, rethrow the caught
error so that it propagates up and the timestamp is captured in the errors
collection. Apply the identical fix to `defi/l2/cli/backfillSingle.ts` lines
52-57 to ensure single-chain backfills also fail visibly when the daily table
write fails.
2 participants
https://github.com/DefiLlama/internal-docs/issues/81
Refactor and migration of the chain assets table:
After the migration we can update #12215 to use fetchDailyChainAssetsCharts instead of fetchHistoricalFromDB
Summary by CodeRabbit
Release Notes
New Features
Chores