Conversation
📝 WalkthroughWalkthroughThe changes add support for 30-day volatility metrics (median APY, standard deviation, and coefficient of variation) to the yield enrichment pipeline. A new Changes
Sequence DiagramsequenceDiagram
participant Handler as Enrichment Handler
participant YieldQueries as Yield Queries Module
participant DB as Database
participant Enricher as Pool Enricher
Handler->>YieldQueries: getVolatility30d() [parallel]
Handler->>YieldQueries: other 30d metrics [parallel]
par Fetch Volatility
YieldQueries->>DB: Query volatility view
DB-->>YieldQueries: Return metrics by configID
and Fetch Other Metrics
YieldQueries->>DB: Query other 30d data
DB-->>YieldQueries: Return metrics
end
YieldQueries-->>Handler: Both metric sets resolved
Handler->>Enricher: Merge metrics into pool data
Enricher-->>Handler: Enriched pools with volatility fields
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ 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
🤖 Fix all issues with AI agents
In `@src/handlers/triggerEnrichment.js`:
- Around line 91-102: The Promise.all call makes the whole enrichment pipeline
fail if getVolatility30d rejects; change to a resilient pattern (e.g., use
Promise.allSettled or catch getVolatility30d separately) so getYieldAvg30d still
resolves and volatilityData defaults to an empty object on error; specifically,
keep calling getYieldAvg30d as before, call getVolatility30d in a way that
captures errors and sets volatilityData = {} on failure, then continue the
dataEnriched = dataEnriched.map(...) step so apyMedian30d/apyStd30d/apyCv30d
fall back to null.
In `@src/queries/yield.js`:
- Around line 321-351: The getVolatility30d function currently returns new
AppError(...) on failure which treats the error as a successful return value;
change this to throw the AppError (or throw a plain Error) so callers can catch
it, and optionally log the failure before throwing; locate the early check in
getVolatility30d (after const response = await conn.query(query)) and replace
the "return new AppError(...)" with throwing the error (e.g., throw new
AppError(...)) so Promise.all consumers and triggerEnrichment.js will receive a
rejection instead of an AppError object.
🧹 Nitpick comments (1)
src/queries/yield.js (1)
325-332: Hardcoded table namevolatilitydeviates from the parameterized pattern used elsewhere.All other queries in this file use
$<table:name>or$<yieldTable:name>for table references, which guards against SQL injection and keeps naming consistent. Consider parameterizing:Suggested fix
const query = ` SELECT "configID", apy_median_30d as "apyMedian30d", apy_std_30d as "apyStd30d", cv_30d as "apyCv30d" - FROM volatility + FROM $<table:name> `; - const response = await conn.query(query); + const response = await conn.query(query, { table: 'volatility' });
| // add 30d avg apy and volatility metrics | ||
| const [avgApy30d, volatilityData] = await Promise.all([ | ||
| getYieldAvg30d(), | ||
| getVolatility30d(), | ||
| ]); | ||
| dataEnriched = dataEnriched.map((p) => ({ | ||
| ...p, | ||
| apyMean30d: avgApy30d[p.configID] ?? null, | ||
| apyMedian30d: volatilityData[p.configID]?.apyMedian30d ?? null, | ||
| apyStd30d: volatilityData[p.configID]?.apyStd30d ?? null, | ||
| apyCv30d: volatilityData[p.configID]?.apyCv30d ?? null, | ||
| })); |
There was a problem hiding this comment.
Consider isolating getVolatility30d failure so it doesn't crash the entire enrichment pipeline.
If the volatility materialized view is missing, stale, or the query fails for any reason, Promise.all will reject and the entire enrichment handler aborts — no pools data gets written to S3. Since volatility metrics are additive/non-critical, a failure there shouldn't take down the whole pipeline.
Suggested resilient approach
- const [avgApy30d, volatilityData] = await Promise.all([
- getYieldAvg30d(),
- getVolatility30d(),
- ]);
+ const [avgApy30d, volatilityData] = await Promise.all([
+ getYieldAvg30d(),
+ getVolatility30d().catch((err) => {
+ console.log('Failed to fetch volatility data, defaulting to empty', err);
+ return {};
+ }),
+ ]);This way, if getVolatility30d fails, volatilityData defaults to {} and all pools simply get null for the three volatility fields via the existing ?? null fallback.
📝 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.
| // add 30d avg apy and volatility metrics | |
| const [avgApy30d, volatilityData] = await Promise.all([ | |
| getYieldAvg30d(), | |
| getVolatility30d(), | |
| ]); | |
| dataEnriched = dataEnriched.map((p) => ({ | |
| ...p, | |
| apyMean30d: avgApy30d[p.configID] ?? null, | |
| apyMedian30d: volatilityData[p.configID]?.apyMedian30d ?? null, | |
| apyStd30d: volatilityData[p.configID]?.apyStd30d ?? null, | |
| apyCv30d: volatilityData[p.configID]?.apyCv30d ?? null, | |
| })); | |
| // add 30d avg apy and volatility metrics | |
| const [avgApy30d, volatilityData] = await Promise.all([ | |
| getYieldAvg30d(), | |
| getVolatility30d().catch((err) => { | |
| console.log('Failed to fetch volatility data, defaulting to empty', err); | |
| return {}; | |
| }), | |
| ]); | |
| dataEnriched = dataEnriched.map((p) => ({ | |
| ...p, | |
| apyMean30d: avgApy30d[p.configID] ?? null, | |
| apyMedian30d: volatilityData[p.configID]?.apyMedian30d ?? null, | |
| apyStd30d: volatilityData[p.configID]?.apyStd30d ?? null, | |
| apyCv30d: volatilityData[p.configID]?.apyCv30d ?? null, | |
| })); |
🤖 Prompt for AI Agents
In `@src/handlers/triggerEnrichment.js` around lines 91 - 102, The Promise.all
call makes the whole enrichment pipeline fail if getVolatility30d rejects;
change to a resilient pattern (e.g., use Promise.allSettled or catch
getVolatility30d separately) so getYieldAvg30d still resolves and volatilityData
defaults to an empty object on error; specifically, keep calling getYieldAvg30d
as before, call getVolatility30d in a way that captures errors and sets
volatilityData = {} on failure, then continue the dataEnriched =
dataEnriched.map(...) step so apyMedian30d/apyStd30d/apyCv30d fall back to null.
| // get 30day volatility metrics from materialized view | ||
| const getVolatility30d = async () => { | ||
| const conn = await connect(); | ||
|
|
||
| const query = ` | ||
| SELECT | ||
| "configID", | ||
| apy_median_30d as "apyMedian30d", | ||
| apy_std_30d as "apyStd30d", | ||
| cv_30d as "apyCv30d" | ||
| FROM volatility | ||
| `; | ||
|
|
||
| const response = await conn.query(query); | ||
|
|
||
| if (!response) { | ||
| return new AppError("Couldn't get volatility data", 404); | ||
| } | ||
|
|
||
| // reformat to object keyed by configID | ||
| const responseObject = {}; | ||
| for (const p of response) { | ||
| responseObject[p.configID] = { | ||
| apyMedian30d: p.apyMedian30d, | ||
| apyStd30d: p.apyStd30d, | ||
| apyCv30d: p.apyCv30d, | ||
| }; | ||
| } | ||
|
|
||
| return responseObject; | ||
| }; |
There was a problem hiding this comment.
return new AppError(...) silently swallows the error instead of propagating it.
This replicates a pre-existing pattern in the file, but it's worth calling out: return new AppError(...) hands an AppError object back to the caller as a successful return value rather than throwing. In triggerEnrichment.js, Promise.all won't catch this, so volatilityData could silently become an AppError instance instead of the expected object.
In practice, this is partially mitigated by the optional chaining (volatilityData[p.configID]?.apyMedian30d ?? null) which will default to null for every pool. However, this means a silent total loss of volatility data with no log or alert.
Consider throwing or logging on error:
Suggested fix
const response = await conn.query(query);
if (!response) {
- return new AppError("Couldn't get volatility data", 404);
+ throw new AppError("Couldn't get volatility data", 404);
}Note: The same return new AppError(...) pattern exists in every other query function in this file. A follow-up to convert them all to throw (or at least add logging) would improve reliability.
📝 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.
| // get 30day volatility metrics from materialized view | |
| const getVolatility30d = async () => { | |
| const conn = await connect(); | |
| const query = ` | |
| SELECT | |
| "configID", | |
| apy_median_30d as "apyMedian30d", | |
| apy_std_30d as "apyStd30d", | |
| cv_30d as "apyCv30d" | |
| FROM volatility | |
| `; | |
| const response = await conn.query(query); | |
| if (!response) { | |
| return new AppError("Couldn't get volatility data", 404); | |
| } | |
| // reformat to object keyed by configID | |
| const responseObject = {}; | |
| for (const p of response) { | |
| responseObject[p.configID] = { | |
| apyMedian30d: p.apyMedian30d, | |
| apyStd30d: p.apyStd30d, | |
| apyCv30d: p.apyCv30d, | |
| }; | |
| } | |
| return responseObject; | |
| }; | |
| // get 30day volatility metrics from materialized view | |
| const getVolatility30d = async () => { | |
| const conn = await connect(); | |
| const query = ` | |
| SELECT | |
| "configID", | |
| apy_median_30d as "apyMedian30d", | |
| apy_std_30d as "apyStd30d", | |
| cv_30d as "apyCv30d" | |
| FROM volatility | |
| `; | |
| const response = await conn.query(query); | |
| if (!response) { | |
| throw new AppError("Couldn't get volatility data", 404); | |
| } | |
| // reformat to object keyed by configID | |
| const responseObject = {}; | |
| for (const p of response) { | |
| responseObject[p.configID] = { | |
| apyMedian30d: p.apyMedian30d, | |
| apyStd30d: p.apyStd30d, | |
| apyCv30d: p.apyCv30d, | |
| }; | |
| } | |
| return responseObject; | |
| }; |
🤖 Prompt for AI Agents
In `@src/queries/yield.js` around lines 321 - 351, The getVolatility30d function
currently returns new AppError(...) on failure which treats the error as a
successful return value; change this to throw the AppError (or throw a plain
Error) so callers can catch it, and optionally log the failure before throwing;
locate the early check in getVolatility30d (after const response = await
conn.query(query)) and replace the "return new AppError(...)" with throwing the
error (e.g., throw new AppError(...)) so Promise.all consumers and
triggerEnrichment.js will receive a rejection instead of an AppError object.
1 participant
Summary by CodeRabbit
New Features
Performance