WIP cache oracle prices #2713
WIP cache oracle prices #2713
Conversation
Co-authored-by: roy-dydx <[email protected]>
…2633) (#2642) Co-authored-by: Adam Fraser <[email protected]>
Co-authored-by: vincentwschau <[email protected]>
Co-authored-by: vincentwschau <[email protected]>
Co-authored-by: vincentwschau <[email protected]>
…ion failure. (backport #2678) (#2698) Co-authored-by: vincentwschau <[email protected]>
…2677) (#2697) Co-authored-by: vincentwschau <[email protected]>
WalkthroughThis pull request makes significant alterations by deprecating the legacy order book mid-price caching system and replacing it with a new Oracle price caching mechanism. Several files related to Redis caching, Lua scripts, and scheduled tasks have been removed or modified. Test suites for the old caching functionality have been eliminated, and the service's initialization code is updated accordingly. Additionally, the GitHub Actions workflow is modified to restrict active configurations to a single branch/job. Changes
Sequence Diagram(s)sequenceDiagram
participant MPH as MarketPriceUpdateHandler
participant OPC as OraclePriceMemoryCache
participant Log as Logger
MPH->>OPC: setOraclePrice(market.pair, oraclePrice.price)
OPC-->>OPC: Store price in oraclePriceCache
OPC-->>MPH: Acknowledge update
MPH->>Log: Log market pair and price update
sequenceDiagram
participant Repo as GitHub Repository
participant Runner as Workflow Runner
participant Job as Build/Push Job (dev2)
Repo->>Runner: Push commit (branch dev2)
Runner->>Job: Trigger active job for dev2
Job-->>Runner: Execute Docker build and push to ECR
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (1)
38-46: Add type for the reduce accumulator.The reduce accumulator type is inferred. Adding an explicit type would improve code maintainability and readability.
Apply this diff to add the type:
+ type AccumulatorType = { + priceCacheKeys: string[]; + priceValues: string[]; + }; + const { priceCacheKeys, priceValues } = cacheKeyPricePairs.reduce( (acc, pair) => { acc.priceCacheKeys.push(pair.cacheKey); acc.priceValues.push(pair.price); return acc; }, - { priceCacheKeys: [] as string[], priceValues: [] as string[] }, + { priceCacheKeys: [], priceValues: [] } as AccumulatorType, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/indexer-build-and-push-dev-staging.yml(2 hunks)indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts(2 hunks)indexer/services/roundtable/src/index.ts(1 hunks)indexer/services/roundtable/src/tasks/market-updater.ts(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/indexer-build-and-push-dev-staging.yml
- indexer/services/roundtable/src/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: test / run_command
- GitHub Check: check-build-auxo
- GitHub Check: run_command
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
| @@ -79,6 +79,8 @@ export default async function runTask(): Promise<void> { | |||
| Date.now() - start, | |||
| ); | |||
|
|
|||
| OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); | |||
There was a problem hiding this comment.
Add error handling for the async cache operation.
The cacheOraclePrices call is not awaited and lacks error handling. This could lead to silent failures and potential inconsistencies in the cache.
Apply this diff to properly handle the async operation:
- OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices);
+ await OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices);📝 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.
| OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); | |
| await OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); |
🛠️ Refactor suggestion
Consider moving the cache operation inside the transaction.
The cache operation is performed outside the transaction block. If the subsequent database updates fail and rollback, the cache will be inconsistent with the database state.
Move the cache operation inside the transaction block (after line 144) to maintain data consistency across Redis and Postgres:
- OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices);
...
await Transaction.commit(txId);
+ await OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices);📝 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.
| OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); | |
| // ... previous lines of code ... | |
| // (Previously at line 82) | |
| // Removed the caching call from here | |
| // OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); | |
| // ... additional code and transaction operations ... | |
| await Transaction.commit(txId); | |
| await OrderbookMidPricesCache.cacheOraclePrices(redisClient, perpetualMarkets, latestPrices); | |
| // ... subsequent code ... |
| export async function cacheOraclePrices( | ||
| client: RedisClient, | ||
| markets: PerpetualMarketFromDatabase[], | ||
| prices: PriceMap, | ||
| ): Promise<void> { | ||
| const cacheKeyPricePairs: | ||
| ({cacheKey: string, price: string})[] = markets.flatMap((market) => { | ||
| const cacheKey: string = getOrderbookMidPriceCacheKey(market.ticker); | ||
| const price = prices[market.marketId]; | ||
|
|
||
| return price !== undefined ? { cacheKey, price } : []; | ||
| }); | ||
|
|
||
| // Extract cache keys and prices | ||
| const { priceCacheKeys, priceValues } = cacheKeyPricePairs.reduce( | ||
| (acc, pair) => { | ||
| acc.priceCacheKeys.push(pair.cacheKey); | ||
| acc.priceValues.push(pair.price); | ||
| return acc; | ||
| }, | ||
| { priceCacheKeys: [] as string[], priceValues: [] as string[] }, | ||
| ); | ||
|
|
||
| const nowSeconds: number = Math.floor(Date.now() / 1000); // Current time in seconds | ||
|
|
||
| logger.info({ | ||
| at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', | ||
| message: 'Caching orderbook mid price', | ||
| cacheKey: priceCacheKeys[0], | ||
| midPrice: priceValues[0], | ||
| }); | ||
|
|
||
| return new Promise<void>((resolve, reject) => { | ||
| client.evalsha( | ||
| addOrderbookMidPricesScript.hash, | ||
| priceCacheKeys.length, | ||
| ...priceCacheKeys, | ||
| ...priceValues, | ||
| nowSeconds, | ||
| (err: Error | null) => { | ||
| if (err) { | ||
| reject(err); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }, | ||
| ); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and refactor to reduce code duplication.
The function lacks validation for empty arrays and shares similar code with fetchAndCacheOrderbookMidPrices. Consider:
- Adding validation for empty arrays
- Extracting shared Redis caching logic into a helper function
Apply this diff to improve the code:
+async function cacheOrderbookPrices(
+ client: RedisClient,
+ priceCacheKeys: string[],
+ priceValues: string[],
+): Promise<void> {
+ if (priceCacheKeys.length === 0) {
+ return;
+ }
+
+ const nowSeconds: number = Math.floor(Date.now() / 1000);
+
+ return new Promise<void>((resolve, reject) => {
+ client.evalsha(
+ addOrderbookMidPricesScript.hash,
+ priceCacheKeys.length,
+ ...priceCacheKeys,
+ ...priceValues,
+ nowSeconds,
+ (err: Error | null) => {
+ if (err) {
+ reject(err);
+ } else {
+ resolve();
+ }
+ },
+ );
+ });
+}
+
export async function cacheOraclePrices(
client: RedisClient,
markets: PerpetualMarketFromDatabase[],
prices: PriceMap,
): Promise<void> {
+ if (markets.length === 0) {
+ return;
+ }
+
const cacheKeyPricePairs:
({cacheKey: string, price: string})[] = markets.flatMap((market) => {
const cacheKey: string = getOrderbookMidPriceCacheKey(market.ticker);
const price = prices[market.marketId];
return price !== undefined ? { cacheKey, price } : [];
});
// Extract cache keys and prices
const { priceCacheKeys, priceValues } = cacheKeyPricePairs.reduce(
(acc, pair) => {
acc.priceCacheKeys.push(pair.cacheKey);
acc.priceValues.push(pair.price);
return acc;
},
{ priceCacheKeys: [] as string[], priceValues: [] as string[] },
);
logger.info({
- at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices',
+ at: 'orderbook-mid-prices-cache#cacheOraclePrices',
message: 'Caching orderbook mid price',
cacheKey: priceCacheKeys[0],
midPrice: priceValues[0],
});
- return new Promise<void>((resolve, reject) => {
- client.evalsha(
- addOrderbookMidPricesScript.hash,
- priceCacheKeys.length,
- ...priceCacheKeys,
- ...priceValues,
- nowSeconds,
- (err: Error | null) => {
- if (err) {
- reject(err);
- } else {
- resolve();
- }
- },
- );
- });
+ return cacheOrderbookPrices(client, priceCacheKeys, priceValues);
}Then update fetchAndCacheOrderbookMidPrices to use the helper function:
});
- return new Promise<void>((resolve, reject) => {
- client.evalsha(
- addOrderbookMidPricesScript.hash,
- priceCacheKeys.length,
- ...priceCacheKeys,
- ...priceValues,
- nowSeconds,
- (err: Error | null) => {
- if (err) {
- reject(err);
- } else {
- resolve();
- }
- },
- );
- });
+ return cacheOrderbookPrices(client, priceCacheKeys, priceValues);
}📝 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.
| export async function cacheOraclePrices( | |
| client: RedisClient, | |
| markets: PerpetualMarketFromDatabase[], | |
| prices: PriceMap, | |
| ): Promise<void> { | |
| const cacheKeyPricePairs: | |
| ({cacheKey: string, price: string})[] = markets.flatMap((market) => { | |
| const cacheKey: string = getOrderbookMidPriceCacheKey(market.ticker); | |
| const price = prices[market.marketId]; | |
| return price !== undefined ? { cacheKey, price } : []; | |
| }); | |
| // Extract cache keys and prices | |
| const { priceCacheKeys, priceValues } = cacheKeyPricePairs.reduce( | |
| (acc, pair) => { | |
| acc.priceCacheKeys.push(pair.cacheKey); | |
| acc.priceValues.push(pair.price); | |
| return acc; | |
| }, | |
| { priceCacheKeys: [] as string[], priceValues: [] as string[] }, | |
| ); | |
| const nowSeconds: number = Math.floor(Date.now() / 1000); // Current time in seconds | |
| logger.info({ | |
| at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', | |
| message: 'Caching orderbook mid price', | |
| cacheKey: priceCacheKeys[0], | |
| midPrice: priceValues[0], | |
| }); | |
| return new Promise<void>((resolve, reject) => { | |
| client.evalsha( | |
| addOrderbookMidPricesScript.hash, | |
| priceCacheKeys.length, | |
| ...priceCacheKeys, | |
| ...priceValues, | |
| nowSeconds, | |
| (err: Error | null) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }, | |
| ); | |
| }); | |
| } | |
| async function cacheOrderbookPrices( | |
| client: RedisClient, | |
| priceCacheKeys: string[], | |
| priceValues: string[], | |
| ): Promise<void> { | |
| if (priceCacheKeys.length === 0) { | |
| return; | |
| } | |
| const nowSeconds: number = Math.floor(Date.now() / 1000); | |
| return new Promise<void>((resolve, reject) => { | |
| client.evalsha( | |
| addOrderbookMidPricesScript.hash, | |
| priceCacheKeys.length, | |
| ...priceCacheKeys, | |
| ...priceValues, | |
| nowSeconds, | |
| (err: Error | null) => { | |
| if (err) { | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }, | |
| ); | |
| }); | |
| } | |
| export async function cacheOraclePrices( | |
| client: RedisClient, | |
| markets: PerpetualMarketFromDatabase[], | |
| prices: PriceMap, | |
| ): Promise<void> { | |
| if (markets.length === 0) { | |
| return; | |
| } | |
| const cacheKeyPricePairs: ({ cacheKey: string, price: string })[] = markets.flatMap((market) => { | |
| const cacheKey: string = getOrderbookMidPriceCacheKey(market.ticker); | |
| const price = prices[market.marketId]; | |
| return price !== undefined ? { cacheKey, price } : []; | |
| }); | |
| // Extract cache keys and prices | |
| const { priceCacheKeys, priceValues } = cacheKeyPricePairs.reduce( | |
| (acc, pair) => { | |
| acc.priceCacheKeys.push(pair.cacheKey); | |
| acc.priceValues.push(pair.price); | |
| return acc; | |
| }, | |
| { priceCacheKeys: [] as string[], priceValues: [] as string[] }, | |
| ); | |
| logger.info({ | |
| at: 'orderbook-mid-prices-cache#cacheOraclePrices', | |
| message: 'Caching orderbook mid price', | |
| cacheKey: priceCacheKeys[0], | |
| midPrice: priceValues[0], | |
| }); | |
| return cacheOrderbookPrices(client, priceCacheKeys, priceValues); | |
| } |
| logger.info({ | ||
| at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', | ||
| message: 'Caching orderbook mid price', | ||
| cacheKey: priceCacheKeys[0], | ||
| midPrice: priceValues[0], | ||
| }); |
There was a problem hiding this comment.
Fix the incorrect logging location identifier.
The logging location identifier is incorrect. It's using the identifier from fetchAndCacheOrderbookMidPrices instead of cacheOraclePrices.
Apply this diff to fix the logging:
logger.info({
- at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices',
+ at: 'orderbook-mid-prices-cache#cacheOraclePrices',
message: 'Caching orderbook mid price',
cacheKey: priceCacheKeys[0],
midPrice: priceValues[0],
});📝 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.
| logger.info({ | |
| at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', | |
| message: 'Caching orderbook mid price', | |
| cacheKey: priceCacheKeys[0], | |
| midPrice: priceValues[0], | |
| }); | |
| logger.info({ | |
| - at: 'orderbook-mid-prices-cache#fetchAndCacheOrderbookMidPrices', | |
| + at: 'orderbook-mid-prices-cache#cacheOraclePrices', | |
| message: 'Caching orderbook mid price', | |
| cacheKey: priceCacheKeys[0], | |
| midPrice: priceValues[0], | |
| }); |
adamfraser
force-pushed
the
oracle-price-candles
branch
from
54f5232 to
2fb8ebc
Compare
adamfraser
force-pushed
the
oracle-price-candles
branch
from
2fb8ebc to
0216755
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (1)
25-73: 🛠️ Refactor suggestionAdd validation and refactor to reduce code duplication.
The function lacks validation for empty arrays and shares similar code with
fetchAndCacheOrderbookMidPrices. Consider:
- Adding validation for empty arrays
- Extracting shared Redis caching logic into a helper function
🧹 Nitpick comments (1)
indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua (1)
5-6: LGTM! Consider adding comments about the fallback value.The change from
ZRANGEtoGETaligns with the simplified caching mechanism. The use offalseas a fallback value for non-existent keys is a good choice as it's explicitly handled in the TypeScript code.Add a comment explaining why
falseis used as the fallback value:local price = redis.call("GET", key) +-- Return false for non-existent keys to distinguish from valid price values results[i] = price or false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/indexer-build-and-push-dev-staging.yml(2 hunks)indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts(4 hunks)indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua(1 hunks)indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua(1 hunks)indexer/services/roundtable/src/index.ts(1 hunks)indexer/services/roundtable/src/tasks/market-updater.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- indexer/services/roundtable/src/tasks/market-updater.ts
- indexer/services/roundtable/src/index.ts
- .github/workflows/indexer-build-and-push-dev-staging.yml
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: test / run_command
- GitHub Check: check-build-auxo
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: check-build-bazooka
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: lint
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: run_command
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: Summary
🔇 Additional comments (1)
indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts (1)
152-193: LGTM! Type changes align with Redis script modifications.The changes in
getMedianPricescorrectly handle the new single-price storage format, and the type updates maintain type safety.
| -- Store the latest price in a simple key-value format without expiration | ||
| redis.call("SET", priceCacheKey, price) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding TTL to prevent stale data.
The script no longer handles expiration, which could lead to stale prices remaining in Redis indefinitely. Consider using SETEX to set a TTL on the cached prices.
Apply this diff to add TTL:
- -- Store the latest price in a simple key-value format without expiration
- redis.call("SET", priceCacheKey, price)
+ -- Store the latest price with a 5-minute TTL to prevent stale data
+ redis.call("SETEX", priceCacheKey, 300, price)📝 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.
| -- Store the latest price in a simple key-value format without expiration | |
| redis.call("SET", priceCacheKey, price) | |
| -- Store the latest price with a 5-minute TTL to prevent stale data | |
| redis.call("SETEX", priceCacheKey, 300, price) |
adamfraser
force-pushed
the
oracle-price-candles
branch
from
0216755 to
4d5c4bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
indexer/services/ender/src/validators/update-perpetual-validator.ts (1)
25-29: Fix type mismatch in createHandlers return type.The method's return type
Handler<UpdatePerpetualEventV1>[]is inconsistent with the class's type parameter which includesUpdatePerpetualEventV2andUpdatePerpetualEventV3. This could lead to runtime errors when handling V2 and V3 events.Apply this diff to fix the type mismatch:
- ): Handler<UpdatePerpetualEventV1>[] { + ): Handler<UpdatePerpetualEventV1 | UpdatePerpetualEventV2 | UpdatePerpetualEventV3>[] {
🧹 Nitpick comments (15)
indexer/services/comlink/src/lib/helpers.ts (1)
726-763: Consider adding error handling for config validation.The vault helper functions look good overall, but there's a potential improvement for the
getVaultPnlStartDatefunction.Consider adding validation for the config value and error handling:
export function getVaultPnlStartDate(): DateTime { + if (!config.VAULT_PNL_START_DATE) { + throw new Error('VAULT_PNL_START_DATE configuration is required'); + } const startDate: DateTime = DateTime.fromISO(config.VAULT_PNL_START_DATE).toUTC(); + if (!startDate.isValid) { + throw new Error(`Invalid VAULT_PNL_START_DATE: ${config.VAULT_PNL_START_DATE}`); + } return startDate; }indexer/packages/postgres/README.md (1)
10-11: Improve documentation clarity.The current wording could be clearer. Consider this revision:
-In `__tests__/db/migrations.test.ts`, test cases may be expected to fail (and hence should be commented out) if a model is modified due to the latest migration. +Note: When modifying database models in the latest migration, certain test cases in `__tests__/db/migrations.test.ts` may fail. These tests should be temporarily disabled until the next migration is added.🧰 Tools
🪛 LanguageTool
[style] ~11-~11: Consider a shorter alternative to avoid wordiness.
Context: ... modified due to the latest migration. In order to migrate in dev and staging, you must re...(IN_ORDER_TO_PREMIUM)
indexer/packages/postgres/src/stores/candle-table.ts (1)
229-236: Consider extracting a helper function to reduce duplication.
You are already performing nearly identical queries for handling different time windows and resolutions. Factoring out a helper could improve maintainability.indexer/services/ender/src/caches/block-cache.ts (1)
124-127: LGTM! Consider adding logging for long initialization times.The performance monitoring implementation is good. Consider adding a warning log when initialization takes longer than expected.
stats.timing( `${config.SERVICE_NAME}.initialize_caches`, Date.now() - start, ); + const duration = Date.now() - start; + if (duration > 5000) { // 5 seconds threshold + logger.warn({ + at: 'block-cache#initializeAllCaches', + message: 'Cache initialization took longer than expected', + durationMs: duration, + }); + }indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (3)
358-358: Returning adjusted PnL ticks
Consider caching or storinggetVaultStartPnl()output locally if it’s called multiple times to reduce overhead.
549-554: RefactoredgetLatestPnlTicks()
Fetches PnL ticks, then adjusts them withgetVaultStartPnl(). Consider adding fallback handling iflatestPnlTicksis empty or if the DB call fails.
568-568: UsinggetVaultStartPnl()for adjustments
No immediate concerns; review performance if the method is repeatedly invoked.indexer/packages/postgres/src/db/migrations/migration_files/20250107145033_default_1hr_funding_for_perp.ts (1)
3-7: Consider specifying precision and scale.Using a plain
decimaltype without explicit precision/scale (e.g.decimal(18,6)) may lead to inconsistent values if extremely large or overly precise entries are inserted. Consider adding explicit constraints to ensure safe ranges for funding rates.indexer/services/roundtable/src/lib/athena-ddl-tables/affiliate_info.ts (1)
8-19: Consider adding field validation for numeric values.The table structure is well-defined, but consider adding validation for numeric fields to ensure data integrity.
For example, you could add validation in the application layer before inserting data:
function validateAffiliateInfo(data: AffiliateInfo): void { if (Big(data.affiliateEarnings).lt(0)) { throw new Error('Affiliate earnings cannot be negative'); } if (data.referredMakerTrades < 0 || data.referredTakerTrades < 0) { throw new Error('Trade counts cannot be negative'); } // Add more validations as needed }indexer/packages/redis/src/caches/next-funding-cache.ts (1)
12-20: Update documentation to reflect new parameter structure.The documentation needs to be updated to describe:
- The new parameter
tickerDefaultFundingRate1HPairs- How the default funding rate is used in the calculation
indexer/packages/postgres/__tests__/helpers/constants.ts (1)
288-288: Document funding rate differences and extract magic values.The different funding rates for cross (0) vs isolated (0.0001) markets should be documented and the values should be extracted into named constants for better maintainability.
Consider applying this improvement:
+// Default funding rates for different market types +const CROSS_MARKET_DEFAULT_FUNDING_RATE = '0'; +const ISOLATED_MARKET_DEFAULT_FUNDING_RATE = '0.0001'; + export const defaultPerpetualMarket: PerpetualMarketCreateObject = { // ... other fields ... - defaultFundingRate1H: '0', + defaultFundingRate1H: CROSS_MARKET_DEFAULT_FUNDING_RATE, // Cross markets have 0 default funding rate }; export const defaultPerpetualMarket2: PerpetualMarketCreateObject = { // ... other fields ... - defaultFundingRate1H: '0', + defaultFundingRate1H: CROSS_MARKET_DEFAULT_FUNDING_RATE, }; export const defaultPerpetualMarket3: PerpetualMarketCreateObject = { // ... other fields ... - defaultFundingRate1H: '0', + defaultFundingRate1H: CROSS_MARKET_DEFAULT_FUNDING_RATE, }; export const isolatedPerpetualMarket: PerpetualMarketCreateObject = { // ... other fields ... - defaultFundingRate1H: '0.0001', + defaultFundingRate1H: ISOLATED_MARKET_DEFAULT_FUNDING_RATE, // Isolated markets have non-zero default funding rate }; export const isolatedPerpetualMarket2: PerpetualMarketCreateObject = { // ... other fields ... - defaultFundingRate1H: '0.0001', + defaultFundingRate1H: ISOLATED_MARKET_DEFAULT_FUNDING_RATE, };Also applies to: 308-308, 328-328, 349-349, 370-370
indexer/services/comlink/public/swagger.json (1)
1141-1144: Review: Validate New FielddefaultFundingRate1HThe new property
"defaultFundingRate1H"has been added to thePerpetualMarketResponseObjectschema with type"string". This follows the convention used for other related price and rate fields. Please verify that representing the funding rate as a string is the intended design—in scenarios where numerical operations might be required, a numeric type could be considered. Also, if this field is critical for downstream consumers, consider whether it should be added to the list of required properties.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (2)
979-979: Consider consolidating duplicated TODO comments.This TODO comment about margin fractions is duplicated across all three versions (V1, V2, V3) of the
expectPerpetualMarketfunctions. Consider extracting it to a shared constant or documentation to maintain DRY principles.
995-995: Consider adding a comment explaining the funding rate calculation.The conversion from 8hr PPM to 1hr rate involves multiple steps that would benefit from a clear explanation.
Add a comment like:
+ // Convert from 8hr PPM rate to 1hr rate: + // 1. Divide by 1_000_000 to convert from PPM + // 2. Divide by 8 to convert from 8hr rate to 1hr rate defaultFundingRate1H: ((perpetual.defaultFunding8hrPpm / 1_000_000) / 8).toString(),indexer/services/ender/__tests__/lib/on-message.test.ts (1)
276-277: Improve type safety in test parameters.The
marketCreateEventparameter is typed asany. This could mask type-related issues and makes the code less maintainable.Consider using a union type:
- marketCreateEvent: any, + marketCreateEvent: PerpetualMarketCreateEventV2 | PerpetualMarketCreateEventV3,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
protocol/indexer/events/events.pb.gois excluded by!**/*.pb.goprotocol/x/perpetuals/types/perpetual.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (82)
.github/workflows/indexer-build-and-push-dev-staging.yml(2 hunks)indexer/packages/postgres/README.md(1 hunks)indexer/packages/postgres/__tests__/db/migrations.test.ts(1 hunks)indexer/packages/postgres/__tests__/helpers/constants.ts(5 hunks)indexer/packages/postgres/src/db/migrations/migration_files/20250107145033_default_1hr_funding_for_perp.ts(1 hunks)indexer/packages/postgres/src/models/perpetual-market-model.ts(3 hunks)indexer/packages/postgres/src/stores/candle-table.ts(2 hunks)indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts(1 hunks)indexer/packages/postgres/src/types/db-model-types.ts(1 hunks)indexer/packages/postgres/src/types/perpetual-market-types.ts(3 hunks)indexer/packages/postgres/src/types/websocket-message-types.ts(1 hunks)indexer/packages/redis/__tests__/caches/next-funding-cache.test.ts(2 hunks)indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts(0 hunks)indexer/packages/redis/src/caches/next-funding-cache.ts(2 hunks)indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts(0 hunks)indexer/packages/redis/src/index.ts(0 hunks)indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua(1 hunks)indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua(1 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts(8 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts(2 hunks)indexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts(3 hunks)indexer/services/comlink/__tests__/lib/request-helpers/request-transformer.test.ts(1 hunks)indexer/services/comlink/public/api-documentation.md(6 hunks)indexer/services/comlink/public/swagger.json(1 hunks)indexer/services/comlink/src/caches/vault-start-pnl.ts(1 hunks)indexer/services/comlink/src/controllers/api/v4/vault-controller.ts(6 hunks)indexer/services/comlink/src/index.ts(2 hunks)indexer/services/comlink/src/lib/compliance-and-geo-check.ts(2 hunks)indexer/services/comlink/src/lib/helpers.ts(4 hunks)indexer/services/comlink/src/request-helpers/request-transformer.ts(1 hunks)indexer/services/comlink/src/types.ts(3 hunks)indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts(0 hunks)indexer/services/ender/__tests__/handlers/funding-handler.test.ts(10 hunks)indexer/services/ender/__tests__/handlers/perpetual-market-handler.test.ts(3 hunks)indexer/services/ender/__tests__/handlers/update-perpetual-handler.test.ts(2 hunks)indexer/services/ender/__tests__/helpers/constants.ts(4 hunks)indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts(2 hunks)indexer/services/ender/__tests__/helpers/notification-functions.test.ts(1 hunks)indexer/services/ender/__tests__/helpers/redis-helpers.ts(1 hunks)indexer/services/ender/__tests__/lib/candles-generator.test.ts(14 hunks)indexer/services/ender/__tests__/lib/on-message.test.ts(6 hunks)indexer/services/ender/__tests__/validators/perpetual-market-validator.test.ts(4 hunks)indexer/services/ender/__tests__/validators/update-perpetual-validator.test.ts(3 hunks)indexer/services/ender/package.json(1 hunks)indexer/services/ender/src/caches/block-cache.ts(2 hunks)indexer/services/ender/src/caches/candle-cache.ts(0 hunks)indexer/services/ender/src/caches/oracle-price-memory-cache.ts(1 hunks)indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts(0 hunks)indexer/services/ender/src/config.ts(1 hunks)indexer/services/ender/src/handlers/markets/market-price-update-handler.ts(2 hunks)indexer/services/ender/src/handlers/perpetual-market-handler.ts(2 hunks)indexer/services/ender/src/helpers/kafka-helper.ts(1 hunks)indexer/services/ender/src/helpers/postgres/postgres-functions.ts(2 hunks)indexer/services/ender/src/index.ts(0 hunks)indexer/services/ender/src/lib/block-processor.ts(1 hunks)indexer/services/ender/src/lib/candles-generator.ts(2 hunks)indexer/services/ender/src/lib/helper.ts(3 hunks)indexer/services/ender/src/lib/types.ts(4 hunks)indexer/services/ender/src/scripts/handlers/dydx_block_processor_ordered_handlers.sql(2 hunks)indexer/services/ender/src/scripts/handlers/dydx_perpetual_market_v3_handler.sql(1 hunks)indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v2_handler.sql(1 hunks)indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v3_handler.sql(1 hunks)indexer/services/ender/src/validators/update-perpetual-validator.ts(1 hunks)indexer/services/roundtable/__tests__/helpers/pnl-ticks-helper.test.ts(1 hunks)indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts(0 hunks)indexer/services/roundtable/__tests__/tasks/market-updater.test.ts(2 hunks)indexer/services/roundtable/src/index.ts(0 hunks)indexer/services/roundtable/src/lib/athena-ddl-tables/affiliate_info.ts(1 hunks)indexer/services/roundtable/src/lib/athena-ddl-tables/affiliate_referred_users.ts(1 hunks)indexer/services/roundtable/src/lib/athena-ddl-tables/perpetual_markets.ts(2 hunks)indexer/services/roundtable/src/lib/athena-ddl-tables/subaccount_usernames.ts(1 hunks)indexer/services/roundtable/src/lib/athena-ddl-tables/vaults.ts(1 hunks)indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts(0 hunks)indexer/services/roundtable/src/tasks/market-updater.ts(2 hunks)indexer/services/roundtable/src/tasks/update-research-environment.ts(3 hunks)indexer/services/socks/__tests__/lib/subscriptions.test.ts(2 hunks)indexer/services/socks/src/helpers/message.ts(1 hunks)indexer/services/socks/src/lib/subscription.ts(1 hunks)indexer/services/socks/src/types.ts(1 hunks)proto/dydxprotocol/indexer/events/events.proto(5 hunks)proto/dydxprotocol/perpetuals/perpetual.proto(1 hunks)protocol/indexer/events/constants.go(1 hunks)
⛔ Files not processed due to max files limit (21)
- protocol/indexer/events/perpetual.go
- protocol/indexer/events/perpetual_market_create.go
- protocol/indexer/events/perpetual_market_create_test.go
- protocol/indexer/events/perpetual_test.go
- protocol/testutil/keeper/clob.go
- protocol/x/clob/abci_test.go
- protocol/x/clob/genesis_test.go
- protocol/x/clob/keeper/clob_pair.go
- protocol/x/clob/keeper/clob_pair_test.go
- protocol/x/clob/keeper/deleveraging_test.go
- protocol/x/clob/keeper/get_price_premium_test.go
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/keeper/msg_server_create_clob_pair_test.go
- protocol/x/clob/keeper/msg_server_place_order_test.go
- protocol/x/clob/keeper/orders_test.go
- protocol/x/clob/keeper/process_operations_test.go
- protocol/x/clob/module_test.go
- protocol/x/listing/keeper/listing.go
- protocol/x/listing/keeper/msg_server_upgrade_isolated_perpetual_to_cross_test.go
- protocol/x/perpetuals/keeper/perpetual.go
- protocol/x/perpetuals/keeper/perpetual_test.go
💤 Files with no reviewable changes (10)
- indexer/services/roundtable/src/index.ts
- indexer/services/ender/src/caches/candle-cache.ts
- indexer/packages/redis/src/index.ts
- indexer/services/roundtable/tests/tasks/cache-orderbook-mid-prices.test.ts
- indexer/packages/redis/tests/caches/orderbook-mid-prices-cache.test.ts
- indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts
- indexer/services/ender/src/index.ts
- indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts
- indexer/services/ender/tests/caches/orderbook-mid-price-memory-cache.test.ts
- indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
✅ Files skipped from review due to trivial changes (5)
- indexer/services/ender/tests/helpers/notification-functions.test.ts
- proto/dydxprotocol/perpetuals/perpetual.proto
- indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v2_handler.sql
- indexer/packages/v4-protos/src/codegen/dydxprotocol/perpetuals/perpetual.ts
- indexer/services/roundtable/tests/helpers/pnl-ticks-helper.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- indexer/services/ender/src/config.ts
- .github/workflows/indexer-build-and-push-dev-staging.yml
- indexer/packages/redis/src/scripts/get_orderbook_mid_prices.lua
- indexer/services/ender/src/handlers/markets/market-price-update-handler.ts
- indexer/services/ender/src/caches/oracle-price-memory-cache.ts
🧰 Additional context used
📓 Learnings (4)
indexer/services/ender/src/scripts/handlers/dydx_block_processor_ordered_handlers.sql (1)
Learnt from: hwray
PR: dydxprotocol/v4-chain#2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
indexer/services/ender/src/helpers/postgres/postgres-functions.ts (2)
Learnt from: hwray
PR: dydxprotocol/v4-chain#2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Learnt from: hwray
PR: dydxprotocol/v4-chain#2597
File: indexer/services/ender/src/helpers/postgres/postgres-functions.ts:47-48
Timestamp: 2024-11-22T18:12:38.778Z
Learning: Prefer to maintain old `v1` handler files, such as `dydx_update_perpetual_handler.sql`, instead of removing them, even when new versioned handlers are added.
indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v3_handler.sql (1)
Learnt from: hwray
PR: dydxprotocol/v4-chain#2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
indexer/services/ender/src/scripts/handlers/dydx_perpetual_market_v3_handler.sql (1)
Learnt from: hwray
PR: dydxprotocol/v4-chain#2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
🪛 Biome (1.9.4)
indexer/services/ender/__tests__/handlers/perpetual-market-handler.test.ts
[error] 106-106: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🪛 LanguageTool
indexer/packages/postgres/README.md
[style] ~11-~11: Consider a shorter alternative to avoid wordiness.
Context: ... modified due to the latest migration. In order to migrate in dev and staging, you must re...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (87)
indexer/services/comlink/src/lib/compliance-and-geo-check.ts (2)
17-20: LGTM!The import statement correctly includes both
create4xxResponseandhandleInternalServerErrorfrom the helpers module.
52-80: LGTM! Robust error handling added.The try-catch block properly handles potential database query errors by calling
handleInternalServerErrorwith appropriate context.indexer/services/comlink/src/lib/helpers.ts (3)
27-27: LGTM!The import statement correctly includes the required types and utilities for vault functionality.
51-51: LGTM!The
VaultMappingtype is correctly imported from the types module.
87-87: LGTM!The
handleInternalServerErrorfunction is now correctly exported for use in other modules.indexer/services/socks/src/helpers/message.ts (1)
12-27: LGTM! Enhanced error message context.The addition of optional
channelandidparameters tocreateErrorMessageimproves error reporting by providing more context about the subscription that failed.indexer/services/socks/src/types.ts (1)
68-72: LGTM! Interface updated to support enhanced error messages.The addition of optional
channelandidproperties to theErrorMessageinterface aligns with the function signature changes and maintains backward compatibility.indexer/services/socks/src/lib/subscription.ts (1)
168-178: LGTM! Enhanced error message with subscription context.The error message now includes the channel and id of the failed subscription, providing better context for debugging.
indexer/services/socks/__tests__/lib/subscriptions.test.ts (2)
247-249: LGTM! Test coverage updated for enhanced error messages.The test expectations are updated to verify that error messages include the channel and id of the failed subscription.
283-285: LGTM! Test coverage for blocked error messages.The test case for blocked subscriptions is updated to verify the enhanced error message format.
indexer/packages/postgres/__tests__/db/migrations.test.ts (2)
37-39: Consider a more robust approach to handling model modifications.While skipping tests that fail due to model modifications is a temporary solution, it might hide potential issues. Consider:
- Adding a configuration flag to conditionally run these tests.
- Implementing test isolation to prevent model changes from affecting other tests.
- Creating separate test suites for model-dependent and model-independent tests.
This would provide better test coverage and make the test suite more maintainable.
Would you like me to propose a more robust testing approach that handles model modifications without skipping tests?
Also applies to: 53-55
26-35: LGTM! Clear and descriptive test name.The new test description "test UP and DOWN for most recent migration without seed data" is more descriptive and helps developers understand the test's purpose.
indexer/services/ender/src/lib/types.ts (3)
10-10: LGTM! New event types imported correctly.The new event types
PerpetualMarketCreateEventV3andUpdatePerpetualEventV3are imported following the existing pattern and naming convention.Also applies to: 36-36
133-139: LGTM! PerpetualMarketCreateEventV3 union case added correctly.The new union case follows the established pattern and includes all required properties.
163-169: LGTM! UpdatePerpetualEventV3 union case added correctly.The new union case follows the established pattern and includes all required properties.
Let's verify the version progression and usage of these new event types:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Verify version progression and usage of the new event types. # Test 1: Check for any skipped versions echo "Checking for version progression..." rg -A 5 "PerpetualMarketCreateEvent[Vv][0-9]+" rg -A 5 "UpdatePerpetualEvent[Vv][0-9]+" # Test 2: Check for handlers of these new event types echo "Checking for handlers..." rg -A 10 "PerpetualMarketCreateEventV3" rg -A 10 "UpdatePerpetualEventV3"Length of output: 80530
Approval: UpdatePerpetualEventV3 union case added successfully.
The version progression and usage for update events have been confirmed. The new event type appears in the expected helper files, protobuf registrations, and test references alongside its V1 and V2 counterparts.
- The update event version (V3) is properly registered and decoded.
- Handlers and tests show appropriate references to UpdatePerpetualEventV3.
indexer/packages/postgres/src/stores/candle-table.ts (5)
4-4: New import looks fine.
No issues found with this added import statement.
178-180: Good early-return check.
Returning an empty object whentickers.length === 0helps avoid unnecessary DB queries.
183-185: Initialization ofcandlesMapis correct.
This ensures each ticker key is primed before further population.
187-207: Potential SQL injection risk with raw query building.
This comment references a prior concern in the review history about constructing SQL strings by interpolating array values. Iftickerscame from untrusted input, it could introduce a vulnerability. It’s safer to use parameter placeholders or a query builder approach.
[duplicate_comment, critical_issue]Suggested fix using parameter placeholders:
- "ticker" IN (${tickers.map((ticker) => { return `'${ticker}'`; }).join(',')}) AND + "ticker" IN (${tickers.map(() => '?').join(',')}) ANDThen supply
tickersas parameters to the.raw(...)call.
208-228: Repeated SQL injection concern.
The same risk applies here. Please consider using parameter placeholders or a query builder method instead of direct string interpolation.
[duplicate_comment, critical_issue]indexer/services/ender/src/caches/block-cache.ts (2)
110-110: LGTM!Good addition of timestamp capture for performance monitoring.
109-128: Verify performance impact across environments.Since this is part of a larger caching system transition, please ensure the performance monitoring is tested across different environments with varying load conditions. This will help establish baseline metrics for the new Oracle price caching system.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check for any existing performance metrics for cache initialization # Search for existing performance monitoring code rg -A 2 "stats\.timing.*initialize_caches" --type tsLength of output: 54
🏁 Script executed:
#!/bin/bash # Display context lines from `indexer/services/ender/src/caches/block-cache.ts` # to verify the presence and context of the performance monitoring code. sed -n '100,140p' indexer/services/ender/src/caches/block-cache.tsLength of output: 1363
Performance monitoring code is in place—please verify its behavior under differing load conditions.
The current implementation in
indexer/services/ender/src/caches/block-cache.tsincludes thestats.timingcall to capture cache initialization metrics. However, as noted, it's important to test these performance measurements in various environments (e.g., staging vs. production) to ensure that the new Oracle price caching system's performance is accurately profiled under realistic load conditions.
- Verify baseline metrics across environments.
- Confirm that any performance degradation or improvement during the caching system transition is captured.
indexer/services/comlink/src/controllers/api/v4/vault-controller.ts (6)
1-1: No issues with newstatsimport
Looks fine.
43-43: Confirmed usage ofgetVaultStartPnl
Import aligns with usage below; no issues found.
112-112: Refined function call
Switching to_.values(vaultSubaccounts)matches the function signature expecting an array of vault objects.
167-167: Updated call to match signature
getLatestPnlTicks()now requires no arguments, aligning with the refactored method.
352-356: Ensure valid return fromgetVaultPnlStartDate()
If there's a possibility of an invalid or undefined date, handle it to avoid runtime errors.
561-565: Parameter alignment inVaultPnlTicksView.getVaultsPnl
Passing interval, window size, and start date is consistent with the new signature.indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (6)
962-963: Ensure callers are updated to use the newer version.Marking the old interface as deprecated is good practice. Make sure all references to
PerpetualMarketCreateEventV2are updated to the newerPerpetualMarketCreateEventV3, to avoid confusion and potential mismatches.
965-966: Coordinate deprecation with downstream consumers.This deprecation notice will help external integrators identify obsolete usage, but a clear timeline and migration guide can further streamline the transition to
PerpetualMarketCreateEventV3.
1041-1042: Confirm all bridging logic is removed or updated.Another deprecation marker. Please double-check whether bridging logic or fallback code references this SDK type. If so, it may need removal or updating.
1111-1262: Review the new PerpetualMarketCreateEventV3 interface consistency.The new V3 interface includes
defaultFunding8hrPpm. Ensure that event producers and consumers handle this field precisely. If the field is truly mandatory, verify that code generating these events provides it correctly, and that consumers won’t break when receiving this attribute.
1496-1497: Validate migration to UpdatePerpetualEventV3.The doc block deprecation calls out V2 as obsolete. Confirm that event emission or consumption code no longer depends on this old version.
1499-1500: Deprecated block references.Adding another deprecated marker is fine, but ensure internal references to
UpdatePerpetualEventV2are fully removed or replaced to avoid confusion.indexer/packages/postgres/src/db/migrations/migration_files/20250107145033_default_1hr_funding_for_perp.ts (1)
9-13: Reverse migration logic looks correct.Dropping the column on
down()ensures a clean rollback. Just be sure no existing data is needed if rolling back.indexer/packages/redis/src/scripts/add_orderbook_mid_prices.lua (1)
17-21: Reiterate TTL addition for preventing stale data.Storing a simple key-value without expiration may lead to outdated cached prices if updates stop. A previous review comment (lines 17-18) suggested adding TTL to mitigate stale data. That recommendation still holds.
indexer/services/roundtable/src/lib/athena-ddl-tables/subaccount_usernames.ts (1)
1-32: LGTM! Well-structured implementation for Athena table creation.The code follows a consistent pattern with proper SQL escaping and clear separation of raw and formatted column definitions.
indexer/services/roundtable/src/lib/athena-ddl-tables/affiliate_referred_users.ts (1)
1-34: LGTM! Well-structured implementation for Athena table creation.The code follows a consistent pattern with proper SQL escaping and appropriate data types (bigint for block numbers).
indexer/services/roundtable/src/lib/athena-ddl-tables/vaults.ts (1)
1-39: LGTM! Well-structured implementation for Athena table creation.The code follows a consistent pattern with proper SQL escaping and appropriate timestamp casting for date fields.
indexer/services/comlink/src/caches/vault-start-pnl.ts (2)
13-21: Add error handling for cache initialization.The
startVaultStartPnlCachefunction should handle potential errors during initialization to prevent silent failures.
27-33: LGTM! Well-implemented test-only function.The
clearVaultStartPnlfunction properly restricts usage to test environment and maintains a clean state.indexer/services/ender/__tests__/helpers/redis-helpers.ts (1)
11-21: LGTM! Changes align with the new caching system.The updated function signature and implementation correctly handle the new funding rate structure with default values.
indexer/services/roundtable/src/lib/athena-ddl-tables/affiliate_info.ts (1)
33-48: LGTM! Well-structured table generation functions.The implementation follows best practices for Athena table creation with proper type casting.
indexer/services/ender/src/handlers/perpetual-market-handler.ts (1)
17-20: LGTM! Clean addition of V3 event support.The changes correctly extend the handler to support the new event version while maintaining backward compatibility.
indexer/services/roundtable/src/lib/athena-ddl-tables/perpetual_markets.ts (1)
25-26: LGTM! The new column is well-integrated.The addition of
defaultFundingRate1Hcolumn is consistent across both table definitions and follows the established pattern of type casting.Also applies to: 45-46
indexer/services/comlink/src/index.ts (1)
8-8: LGTM! Cache initialization is properly integrated.The vault start PnL cache initialization follows the existing pattern and is properly awaited.
Also applies to: 46-47
indexer/packages/redis/src/caches/next-funding-cache.ts (1)
21-47: LGTM! The funding rate calculation is properly updated.The changes correctly:
- Accept an array of tuples containing ticker and default funding rate
- Add the default funding rate to the calculated average
indexer/packages/redis/__tests__/caches/next-funding-cache.test.ts (1)
20-28: LGTM! Test cases are comprehensive and well-documented.The test cases:
- Cover the new default funding rate behavior
- Include edge cases
- Have clear comments explaining calculations
Also applies to: 36-44, 48-54
indexer/packages/postgres/src/types/perpetual-market-types.ts (1)
21-21: Verify the type and add documentation for the new funding rate field.The new
defaultFundingRate1Hfield is added consistently across interfaces and enums, but it would benefit from documentation explaining:
- The expected format and range of values
- The units of measurement (e.g., percentage, basis points)
- Any validation requirements
Also applies to: 40-40, 59-59
protocol/indexer/events/constants.go (1)
35-35: Verify version synchronization with indexer service.The version increments for
PerpetualMarketEventVersionandUpdatePerpetualEventVersionshould be synchronized with the corresponding versions in the indexer service.Also applies to: 37-37
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check version synchronization between protocol and indexer # Test: Search for version constants in indexer code rg -A 1 'PerpetualMarketEventVersion|UpdatePerpetualEventVersion' indexer/Length of output: 76
🏁 Script executed:
#!/bin/bash # Description: Search entire repository for version constant definitions to verify synchronization between protocol and indexer service. echo "Searching for PerpetualMarketEventVersion:" rg "PerpetualMarketEventVersion" . echo "Searching for UpdatePerpetualEventVersion:" rg "UpdatePerpetualEventVersion" .Length of output: 3585
Version constants are synchronized with the indexer service.
The repository inspection confirms that both
PerpetualMarketEventVersionandUpdatePerpetualEventVersionare set to3inprotocol/indexer/events/constants.goand referenced consistently across tests and keeper implementations.indexer/packages/postgres/src/stores/vault-pnl-ticks-view.ts (2)
66-88: Add error handling to the new function.The new
getLatestVaultPnlfunction should handle potential database errors consistently with other functions in the codebase.
71-82: LGTM! Efficient query implementation.The SQL query efficiently retrieves the latest PnL for each subaccount using:
- DISTINCT ON to get unique subaccount records
- Correct ordering by blockTime DESC to get the most recent records
indexer/packages/postgres/src/models/perpetual-market-model.ts (1)
89-89: LGTM! The new field is well-defined and properly integrated.The
defaultFundingRate1Hfield is correctly added to the model with:
- Proper type validation in jsonSchema
- Correct SQL to JSON conversion mapping
- Optional field in the class definition
Also applies to: 119-119, 157-157
indexer/services/ender/src/helpers/postgres/postgres-functions.ts (1)
41-41: LGTM! Handler scripts are properly versioned.The new V3 handler scripts are added while maintaining the existing V1 and V2 handlers, which aligns with the best practice of maintaining backward compatibility.
Also applies to: 50-50
indexer/services/ender/__tests__/validators/perpetual-market-validator.test.ts (1)
5-5: LGTM! Test coverage is properly extended for V3 events.The test suite is correctly updated to include V3 events while maintaining existing test coverage for V1 and V2 events.
Also applies to: 16-16, 54-59, 64-65, 95-96, 104-105, 113-114, 119-120
indexer/services/ender/__tests__/handlers/update-perpetual-handler.test.ts (1)
17-17: LGTM! Test coverage is properly extended for V3 events.The test suite is correctly updated to include V3 events while maintaining existing test coverage for V1 and V2 events.
Also applies to: 25-25, 76-80, 84-85
indexer/services/roundtable/src/tasks/market-updater.ts (1)
55-62: LGTM! The new data structure enhances funding rate management.The change from
tickerstotickerDefaultFundingRate1HPairsimproves the handling of funding rates by including default rates, with a safe fallback value of '0' for null rates.indexer/packages/postgres/src/types/websocket-message-types.ts (1)
220-220: LGTM! The interface update is consistent with the funding rate enhancements.The addition of
defaultFundingRate1Has an optional property is correctly placed in the "Fields that are very unlikely to change" section.indexer/services/ender/__tests__/handlers/perpetual-market-handler.test.ts (2)
95-101: LGTM! Test coverage extended for V3 events.The test suite is correctly updated to include support for
PerpetualMarketCreateEventV3.
106-106: 🛠️ Refactor suggestionReplace
Functiontype with explicit function signature.Using the
Functiontype is discouraged as it accepts any function-like value, which can lead to type-related bugs.Replace with an explicit function signature:
-expectPerpetualMarket: Function, +expectPerpetualMarket: ( + perpetualMarket: PerpetualMarketFromDatabase, + event: PerpetualMarketCreateEventV1 | PerpetualMarketCreateEventV2 | PerpetualMarketCreateEventV3 +) => void,Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
indexer/packages/postgres/src/types/db-model-types.ts (1)
98-98: LGTM! The interface update aligns with the funding rate enhancements.The addition of
defaultFundingRate1Has an optional property is consistent with the changes across the codebase.indexer/services/roundtable/src/tasks/update-research-environment.ts (1)
21-22: LGTM! New Athena tables added correctly.The implementation follows the existing pattern for adding new tables to Athena.
Also applies to: 36-36, 42-42, 65-68
indexer/services/ender/src/helpers/kafka-helper.ts (1)
350-350: LGTM! New funding rate field added correctly.The
defaultFundingRate1Hfield is correctly added to the market message contents.indexer/services/ender/src/lib/helper.ts (1)
176-183: LGTM! Version 3 event support added correctly.The implementation properly handles the new event versions while maintaining backward compatibility.
Also applies to: 228-235
indexer/services/comlink/__tests__/lib/request-helpers/request-transformer.test.ts (1)
84-84: LGTM! Test expectations updated correctly.The test has been properly updated to include the new
defaultFundingRate1Hfield in the expected response object.indexer/services/ender/src/lib/block-processor.ts (1)
52-52: LGTM! Added support for version 3 events.The changes correctly extend the validator mappings to support version 3 of
PERPETUAL_MARKETandUPDATE_PERPETUALsubtypes, following the existing pattern.Also applies to: 56-56
indexer/services/ender/__tests__/handlers/funding-handler.test.ts (1)
15-15: LGTM! Added test cases for funding rate calculation.The changes improve test coverage by:
- Testing non-zero and zero funding values
- Verifying the next funding rate calculation with different default funding rates
- Creating test markets with specific configurations
Also applies to: 46-46, 139-191
indexer/services/ender/__tests__/helpers/constants.ts (1)
29-30: LGTM! Added test constants for version 3 events.The changes add test constants that:
- Follow the existing pattern of defining test data
- Include the new
defaultFunding8hrPpmfield- Test different market types (CROSS vs ISOLATED)
Also applies to: 40-40, 165-193, 244-252
indexer/services/comlink/src/types.ts (1)
27-27: LGTM! Enhanced type definitions.The changes:
- Add type-safe mapping for vault data
- Add optional funding rate field to market response
- Follow TypeScript best practices
Also applies to: 355-355, 696-698
indexer/services/ender/src/lib/candles-generator.ts (2)
29-29: LGTM! Importing the new oracle price cache function.The change from orderbook mid-price to oracle price cache aligns with the architectural shift in price data management.
541-541: Verify the impact of switching to oracle prices.The switch from orderbook mid-prices to oracle prices for the price map could affect:
- Price accuracy - Oracle prices might have different update frequencies
- System performance - Different caching mechanisms could affect latency
- Market data consistency - Ensure all dependent systems are updated to use oracle prices
Run the following script to verify the usage of oracle prices:
indexer/services/comlink/src/request-helpers/request-transformer.ts (1)
392-392: LGTM! Adding defaultFundingRate1H to the response object.The addition of
defaultFundingRate1Henhances the API response with funding rate information.indexer/services/roundtable/__tests__/tasks/market-updater.test.ts (3)
147-147: LGTM! Clarifying test case description.The rename from "funding rates" to "funding samples" better reflects the actual test scenario.
384-408: LGTM! Adding test coverage for non-zero default funding rates.The test ensures that no message is sent when there are no funding samples, even with non-zero default funding rates.
409-484: LGTM! Adding test coverage for zero funding samples.The test verifies that a message is sent when:
- The funding sample is zero
- The
nextFundingRatediffers from the current rateindexer/services/comlink/__tests__/controllers/api/v4/vault-controller.test.ts (3)
27-27: LGTM! Importing vault start PNL cache functions.The import adds necessary functions for managing the vault start PNL cache state in tests.
135-135: LGTM! Clearing vault start PNL cache between tests.Properly isolates tests by clearing the cache state in afterEach.
658-658: LGTM! Initializing vault start PNL cache after creating PNL ticks.Ensures the cache is properly initialized with the created PNL ticks data.
indexer/services/ender/__tests__/lib/candles-generator.test.ts (1)
38-39: LGTM! Good improvements in DateTime handling and price caching.The changes improve the code by:
- Using luxon's DateTime for consistent time handling
- Implementing proper oracle price caching
- Maintaining clear test structure
indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v3_handler.sql (1)
1-47: SQL Update Handler Implementation Looks Good
The implementation ofdydx_update_perpetual_v3_handleris well-documented and follows a clear, maintainable structure. The use of constants (e.g.,PPM_EXPONENTandFUNDING_RATE_FROM_PROTOCOL_IN_HOURS) and the calculation ofdefaultFundingRate1Hviadydx_trim_scaleis both clear and concise. Make sure that the expected types withinevent_dataare validated upstream to avoid conversion issues.indexer/services/ender/src/scripts/handlers/dydx_perpetual_market_v3_handler.sql (1)
1-46: SQL Market Creation Handler (V3) is Well-Structured
Thedydx_perpetual_market_v3_handlerfunction is neatly implemented. Field extraction and conversions (including the use ofdydx_from_jsonlib_longanddydx_trim_scale) are consistent with the project’s practices. The use of anINSERTwith record expansion simplifies the insertion logic. Just ensure that the structure ofevent_dataalways provides the values expected by the target table schema.indexer/services/ender/src/scripts/handlers/dydx_block_processor_ordered_handlers.sql (2)
59-61: Added Support for Perpetual Market Event Version 3
The new case for"perpetual_market"events (version 3) correctly routes the events todydx_perpetual_market_v3_handler(event_data). Please verify that this branch is covered by tests and that the handler’s output integrates smoothly with consumers downstream.
72-73: Enhanced Update Perpetual Handling for Version 3
Adding a new clause for"update_perpetual"events (version 3) that callsdydx_update_perpetual_v3_handler(event_data)extends the function’s functionality without impacting legacy logic. Confirm that existing handlers (for versions 1 and 2) are still functioning as expected and that tests have been updated to cover all cases.proto/dydxprotocol/indexer/events/events.proto (2)
377-485: Update to Perpetual Market Create Events – Introducing V3
The introduction ofPerpetualMarketCreateEventV3with the new fielddefault_funding8hr_ppmprovides enhanced detail for funding rate information. Ensure that downstream consumers and documentation are updated to clarify the differences between this field and the legacy funding rate representations (e.g., the SQL function’sdefaultFundingRate1H).
577-641: Deprecation and Introduction of Update Perpetual Event V3
DeprecatingUpdatePerpetualEventV2in favor of the newUpdatePerpetualEventV3(which now includes additional fields such astickerand thedefault_funding8hr_ppm) modernizes the event structure. Verify that all protocol consumers know about this change and that necessary migrations or version-handling logic are in place.indexer/services/comlink/public/api-documentation.md (2)
2498-2499: Clarify the new field description fordefaultFundingRate1H.In the first market object (property1) the diff adds the following two lines:
+ "baseOpenInterest": "string", + "defaultFundingRate1H": "string"While the syntax is correct and the field is consistently added, it would be beneficial to provide a clear description for both
baseOpenInterestand especially the newdefaultFundingRate1Hfield. This additional detail will help API consumers understand the meaning and usage of the new funding rate information.
2522-2523: Ensure consistency in the market object schema.In the second market object (property2) you have similarly added:
+ "baseOpenInterest": "string", + "defaultFundingRate1H": "string"This mirrors the changes in the first market object. Please double-check that the corresponding OpenAPI/Swagger schema (if applicable) and any supporting documentation (or developer guides) are updated accordingly so that the new field is documented consistently across all parts of the API.
✅ Verification successful
Error: No shell scripts or web queries found
🏁 Script executed:
#!/bin/bash # Search across the repository to verify that the new fields in the market object # ("baseOpenInterest" and "defaultFundingRate1H") are documented consistently # alongside the existing fields (like "maintenanceMarginFraction" and "openInterest"). echo "Searching for 'baseOpenInterest'" rg "baseOpenInterest" echo "Searching for 'defaultFundingRate1H'" rg "defaultFundingRate1H" echo "Searching for 'maintenanceMarginFraction'" rg "maintenanceMarginFraction" --context 3 echo "Searching for 'openInterest'" rg "openInterest" --context 3Length of output: 66386
Consistent Documentation Verified
The search results confirm that the new properties (
baseOpenInterestanddefaultFundingRate1H) are documented in the same manner as the other market object fields. They appear consistently in both the OpenAPI/Swagger schema (inswagger.json) and supporting documentation (inapi-documentation.md), as well as in the relevant TypeScript types and tests.
- The new fields are defined as
"string"in the API docs.- Corresponding definitions exist in the Swagger schema and tests, mirroring the implementation of other market object fields.
No discrepancies were found across the API documentation.
| @@ -7,6 +7,8 @@ Add a knex migration by running `pnpm run migrate:make <create_fake_table>` | |||
|
|
|||
| Run the migration with `pnpm run migrate` | |||
|
|
|||
| In `__tests__/db/migrations.test.ts`, test cases may be expected to fail (and hence should be commented out) if a model is modified due to the latest migration. | |||
|
|
|||
| In order to migrate in dev and staging, you must redeploy and run bazooka. | |||
|
|
|||
| TODO(CORE-512): Add info/resources around bazooka. [Doc](https://www.notion.so/dydx/Engineering-Runbook-15064661da9643188ce33e341b68e7bb#cb2283d80ef14a51924f3bd1a538fd82). | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Follow up on documentation TODO.
The TODO item (CORE-512) indicates missing documentation about the bazooka deployment tool. Since this is a WIP PR, please ensure this documentation is completed before merging.
Run this script to check the status of the TODO item:
🏁 Script executed:
#!/bin/bash
# Description: Check if the TODO item is still open
gh issue view CORE-512 2>/dev/null || echo "Issue not found"Length of output: 65
Documentation Follow-Up: CORE-512 Issue is Missing
- The shell script output indicates that GitHub issue CORE-512 does not exist.
- Confirm whether the documentation for the bazooka deployment tool has been completed.
- If the documentation work is still pending, please either create a new issue to track it or update/remove the corresponding TODO in the code.
| /** | ||
| * UpdatePerpetualEventV3 message contains all the information about an update | ||
| * to a perpetual on the dYdX chain. | ||
| */ | ||
|
|
||
| export interface UpdatePerpetualEventV3 { | ||
| /** | ||
| * Unique Perpetual id. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
| id: number; | ||
| /** | ||
| * The name of the `Perpetual` (e.g. `BTC-USD`). | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| ticker: string; | ||
| /** | ||
| * Unique id of market param associated with this perpetual market. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| marketId: number; | ||
| /** | ||
| * The exponent for converting an atomic amount (`size = 1`) | ||
| * to a full coin. For example, if `AtomicResolution = -8` | ||
| * then a `PerpetualPosition` with `size = 1e8` is equivalent to | ||
| * a position size of one full coin. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| atomicResolution: number; | ||
| /** | ||
| * The liquidity_tier that this perpetual is associated with. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| liquidityTier: number; | ||
| /** Market type of the perpetual. */ | ||
|
|
||
| marketType: PerpetualMarketType; | ||
| /** Default 8hr funding rate in parts-per-million. */ | ||
|
|
||
| defaultFunding8hrPpm: number; | ||
| } | ||
| /** | ||
| * UpdatePerpetualEventV3 message contains all the information about an update | ||
| * to a perpetual on the dYdX chain. | ||
| */ | ||
|
|
||
| export interface UpdatePerpetualEventV3SDKType { | ||
| /** | ||
| * Unique Perpetual id. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
| id: number; | ||
| /** | ||
| * The name of the `Perpetual` (e.g. `BTC-USD`). | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| ticker: string; | ||
| /** | ||
| * Unique id of market param associated with this perpetual market. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| market_id: number; | ||
| /** | ||
| * The exponent for converting an atomic amount (`size = 1`) | ||
| * to a full coin. For example, if `AtomicResolution = -8` | ||
| * then a `PerpetualPosition` with `size = 1e8` is equivalent to | ||
| * a position size of one full coin. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| atomic_resolution: number; | ||
| /** | ||
| * The liquidity_tier that this perpetual is associated with. | ||
| * Defined in perpetuals.perpetual | ||
| */ | ||
|
|
||
| liquidity_tier: number; | ||
| /** Market type of the perpetual. */ | ||
|
|
||
| market_type: PerpetualMarketTypeSDKType; | ||
| /** Default 8hr funding rate in parts-per-million. */ | ||
|
|
||
| default_funding8hr_ppm: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check the new UpdatePerpetualEventV3 usage carefully.
The addition of defaultFunding8hrPpm can introduce breaking changes for older logic. Confirm that all related migrations, database updates, and event-consumer code paths are aligned with the new attribute.
Do you want me to generate compatibility checks or backfill logic for older records so that the new field is never null or inaccurately populated?
| import { getVaultMapping, getVaultPnlStartDate } from '../lib/helpers'; | ||
| import { VaultMapping } from '../types'; | ||
|
|
||
| let vaultStartPnl: PnlTicksFromDatabase[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add initialization check in getter function.
The getVaultStartPnl function should verify that the cache has been initialized before returning the value.
+let initialized = false;
let vaultStartPnl: PnlTicksFromDatabase[] = [];
export function getVaultStartPnl(): PnlTicksFromDatabase[] {
+ if (!initialized) {
+ throw new Error('Vault start PnL cache has not been initialized');
+ }
return vaultStartPnl;
}Also applies to: 23-25
| [ | ||
| 'UpdatePerpetualEventV3', | ||
| UpdatePerpetualEventV3.encode(defaultUpdatePerpetualEventV3).finish(), | ||
| defaultUpdatePerpetualEventV3, | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify test coverage for V3-specific fields.
While the basic validation is tested, consider adding test cases specific to the new fields in UpdatePerpetualEventV3 (e.g., defaultFundingRate1H).
Also applies to: 68-68
🏁 Script executed:
#!/bin/bash
# Description: Check test coverage for V3-specific fields
# Test: Search for test cases related to new fields in V3
rg -A 5 'defaultFundingRate1H.*test' indexer/services/ender/__tests__/Length of output: 72
I'll run additional searches to verify if there are any explicit tests for V3-specific fields (like defaultFundingRate1H). In particular, I'm looking for any occurrences of this field or related funding rate terminology in the tests (and ensuring the V3 tests reference all relevant fields). Please execute the following script:
This will help confirm if additional test cases or validations for defaultFundingRate1H (and possibly other V3-specific fields) are present.
🏁 Script executed:
#!/bin/bash
echo "Searching for 'defaultFundingRate1H' in test files:"
rg -n "defaultFundingRate1H" indexer/services/ender/__tests__/
echo "Searching for any 'fundingRate' references for broader context:"
rg -n "fundingRate" indexer/services/ender/__tests__/
echo "Searching for 'UpdatePerpetualEventV3' references in test files:"
rg -n "UpdatePerpetualEventV3" indexer/services/ender/__tests__/Length of output: 3278
Enhance V3-specific field validation in tests
The current tests for UpdatePerpetualEventV3 (in update-perpetual-validator.test.ts) validate the basic structure via encoding/decoding, but explicit assertions for V3-specific fields—such as defaultFundingRate1H—are missing. While this field is exercised across helper and funding-handler tests, it isn’t directly validated in the V3 validator test cases. It’s recommended to add explicit test cases for these new fields to ensure their intended behavior is fully covered.
- Location: indexer/services/ender/tests/validators/update-perpetual-validator.test.ts (lines 60–64)
| @@ -7,7 +7,7 @@ | |||
| "start": "node --heapsnapshot-signal=SIGUSR2 -r dd-trace/init -r dotenv-flow/config build/src/index.js", | |||
| "build": "rm -rf build/ && tsc && cp -r src/scripts build/src/scripts", | |||
| "build:prod": "pnpm run build", | |||
| "build:watch": "pnpm run build -- --watch", | |||
| "build:watch": "rm -rf build/ && tsc --watch && cp -r src/scripts build/src/scripts", | |||
There was a problem hiding this comment.
Review Build:watch Script – Consider Concurrency for File Copy
The new "build:watch" command now cleans the build directory and then runs tsc --watch followed by copying scripts. However, since tsc --watch is a long-running (blocking) command, the subsequent cp -r src/scripts build/src/scripts might never execute. Consider using a tool like concurrently (or npm-run-all) to run the watcher and the copy step in parallel, or separate the copy task into a pre‑build/post‑build step.
adamfraser
force-pushed
the
oracle-price-candles
branch
from
78cef84 to
fa9e6b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
indexer/services/ender/src/lib/candles-generator.ts (2)
532-546:🧩 Analysis chain
Rename function and update related documentation to reflect oracle price usage.
The function now returns oracle prices instead of orderbook mid prices, but the name and documentation haven't been updated to reflect this change.
Apply these changes:
-/** - * Get the cached orderbook mid price for a given ticker -*/ +/** + * Get the cached oracle price for a given ticker +*/ -export function getOrderbookMidPriceMap(): { [ticker: string]: OrderbookMidPrice } { +export function getOraclePriceMap(): { [ticker: string]: OrderbookMidPrice } { const start: number = Date.now(); const perpetualMarkets = Object.values(perpetualMarketRefresher.getPerpetualMarketsMap()); const priceMap: { [ticker: string]: OrderbookMidPrice } = {}; perpetualMarkets.forEach((perpetualMarket: PerpetualMarketFromDatabase) => { priceMap[perpetualMarket.ticker] = getOracleCachePrice(perpetualMarket.ticker); }); - stats.timing(`${config.SERVICE_NAME}.get_orderbook_mid_price_map.timing`, Date.now() - start); + stats.timing(`${config.SERVICE_NAME}.get_oracle_price_map.timing`, Date.now() - start); return priceMap; }Additionally, update all references to this function in the codebase.
🏁 Script executed:
#!/bin/bash # Find all references to getOrderbookMidPriceMap in the codebase rg "getOrderbookMidPriceMap"Length of output: 539
Rename function and update documentation & references for oracle pricing.
The code now returns oracle prices, but the function name, its JSDoc, and the stats metric still reference "orderbook mid price." Additionally, the test files are still importing and using
getOrderbookMidPriceMap. You should:
- Rename the function (e.g., to
getOraclePriceMap) and update its JSDoc.- Update the timing metric name accordingly.
- Update all codebase references (including in tests such as
indexer/services/ender/__tests__/lib/candles-generator.test.ts) to use the new function name.
374-388:🧩 Analysis chain
Consider updating database schema to reflect oracle price usage.
The code now uses oracle prices, but the database columns and variable names still reference orderbook mid prices (e.g.,
orderbookMidPriceClose,orderbookMidPriceOpen). Consider creating a follow-up task to:
- Create new columns for oracle prices
- Migrate existing data
- Update all references in the code
- Remove old columns
This should be done as a separate PR to ensure safe data migration.
Also applies to: 406-420, 480-490
🏁 Script executed:
#!/bin/bash # Find all references to orderbookMidPrice in the codebase rg "orderbookMidPrice" # Find database migration files fd -e sqlLength of output: 16569
Follow-Up: Address Inconsistent Usage of Orderbook Mid Price Naming
The codebase still widely references “orderbookMidPrice” (and its variants) in the candles generator, tests, API docs, and even in migration scripts. Given that we’re transitioning to oracle prices, a follow-up task is needed to update these references and related database schema. For example:
File updates:
- Update variable names in
indexer/services/ender/src/lib/candles-generator.tsto use oracle price naming.- Modify test files (e.g.,
indexer/services/ender/__tests__/lib/candles-generator.test.ts) to reflect the new naming conventions.Database/schema updates:
- Create new columns for oracle prices and migrate existing data.
- Update migration files (e.g., in
indexer/packages/postgres/src/db/migrations/migration_files/20240627152937_candles_add_mid_book_price_open_and_close.ts) to handle the intended removal or replacement of the old columns.Documentation updates:
- Ensure API documentation and related files (e.g.,
indexer/services/comlink/public/api-documentation.mdandswagger.json) are updated accordingly.A separate PR should be scheduled to safely transition the schema and code to properly accommodate the oracle price usage.
🧹 Nitpick comments (2)
indexer/services/roundtable/src/tasks/market-updater.ts (1)
55-62: LGTM! Consider adding type safety.The implementation correctly handles null values and includes clear documentation. For better type safety, consider defining an interface for the tuple type.
+type TickerFundingRatePair = [string, string]; + const tickerDefaultFundingRate1HPairs: [string, string][] = _.map(indexer/services/ender/__tests__/lib/candles-generator.test.ts (1)
676-689: Enhance test coverage for the orderbook price map.The new test case verifies the basic functionality of
getOrderbookMidPriceMap, but consider adding test cases for:
- Empty cache scenario
- Invalid price formats
- Price updates
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/indexer-build-and-push-dev-staging.yml(2 hunks).github/workflows/indexer-build-and-push-mainnet.yml(1 hunks)indexer/packages/redis/__tests__/caches/orderbook-mid-prices-cache.test.ts(0 hunks)indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts(0 hunks)indexer/packages/redis/src/index.ts(0 hunks)indexer/services/ender/__tests__/caches/orderbook-mid-price-memory-cache.test.ts(0 hunks)indexer/services/ender/__tests__/lib/candles-generator.test.ts(14 hunks)indexer/services/ender/src/caches/oracle-price-memory-cache.ts(1 hunks)indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts(0 hunks)indexer/services/ender/src/handlers/markets/market-price-update-handler.ts(2 hunks)indexer/services/ender/src/index.ts(0 hunks)indexer/services/ender/src/lib/candles-generator.ts(2 hunks)indexer/services/roundtable/__tests__/tasks/cache-orderbook-mid-prices.test.ts(0 hunks)indexer/services/roundtable/src/index.ts(0 hunks)indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts(0 hunks)indexer/services/roundtable/src/tasks/market-updater.ts(2 hunks)
💤 Files with no reviewable changes (9)
- indexer/packages/redis/src/index.ts
- indexer/services/roundtable/src/index.ts
- indexer/services/ender/src/index.ts
- indexer/services/roundtable/src/tasks/cache-orderbook-mid-prices.ts
- indexer/services/ender/tests/caches/orderbook-mid-price-memory-cache.test.ts
- indexer/services/roundtable/tests/tasks/cache-orderbook-mid-prices.test.ts
- indexer/services/ender/src/caches/orderbook-mid-price-memory-cache.ts
- indexer/packages/redis/tests/caches/orderbook-mid-prices-cache.test.ts
- indexer/packages/redis/src/caches/orderbook-mid-prices-cache.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/indexer-build-and-push-dev-staging.yml
- indexer/services/ender/src/handlers/markets/market-price-update-handler.ts
- indexer/services/ender/src/caches/oracle-price-memory-cache.ts
⏰ Context from checks skipped due to timeout of 90000ms (24)
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Staging) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: Analyze (go)
- GitHub Check: Summary
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Dev2) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
🔇 Additional comments (8)
.github/workflows/indexer-build-and-push-mainnet.yml (1)
9-9: New Branch Trigger for 'oracle-price-candles' AddedThe addition of the
'oracle-price-candles'branch in the push trigger is clear and aligns with the objective to include Oracle price caching updates. Ensure that all subsequent processes (e.g., build and push jobs) are compatible with changes deployed via this new branch.indexer/services/roundtable/src/tasks/market-updater.ts (2)
80-80: LGTM! Cache function call is properly updated.The NextFundingCache.getNextFunding call is correctly updated to use the new parameter structure and is properly awaited within Promise.all.
109-152: Consider moving the cache operation inside the transaction block.The cache operation (getNextFunding) is performed outside the transaction block. If the subsequent database updates fail and rollback, the cache will be inconsistent with the database state.
Move the cache operation inside the transaction block to maintain data consistency across Redis and Postgres:
const txId: number = await Transaction.start(); try { + const fundingRates = await NextFundingCache.getNextFunding( + redisClient, + tickerDefaultFundingRate1HPairs, + ); await PerpetualMarketTable.updateMarketCheckerFields(indexer/services/ender/src/lib/candles-generator.ts (1)
29-29: LGTM!The import for
getOracleCachePriceis correctly placed with other cache-related imports.indexer/services/ender/__tests__/lib/candles-generator.test.ts (4)
38-39: LGTM: Import changes align with the new oracle price caching mechanism.The addition of
DateTimefromluxonandsetOraclePricefrom the oracle price memory cache reflects the transition from orderbook mid-price caching to oracle price caching.
73-75: LGTM: Helper function updated to use oracle price.The
setCachePricehelper function has been correctly updated to usesetOraclePriceinstead of the deprecated orderbook mid-price cache.
145-146: Verify oracle price consistency across test cases.The test cases have been updated to use oracle prices instead of orderbook mid-prices. However, ensure that the expected values are consistent with the new oracle price mechanism:
- Some test cases expect
nullvalues (lines 377-378)- Others expect specific price values
- Some test cases expect
undefinedvaluesAlso applies to: 191-192, 316-317, 346-347, 377-378, 539-540, 568-569, 667-668
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all oracle price related assertions rg -A 2 'orderbookMidPrice(Open|Close).*[:=]'Length of output: 12364
Oracle Price Consistency Verified
The test cases clearly account for all three scenarios—explicit numeric values (e.g., '110000', '80500'),
null, andundefined—which aligns with the new oracle price mechanism. No discrepancies were observed in the expected values.
- The test at indexer/services/ender/tests/lib/candles-generator.test.ts (lines 145-146) sets explicit oracle price values.
- Other test cases correctly expect
nullorundefinedbased on specific conditions.
42-45: Verify the time handling logic.The test setup now uses
DateTimefor time calculations and mocks. Ensure that:
- The time calculations are consistent across all test cases
- The mock time reset in
afterEachproperly restores the system timeAlso applies to: 56-57, 64-64
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for all DateTime usage to verify consistency rg -A 2 'DateTime\.'Length of output: 78160
Time Handling Logic Verified
After reviewing the DateTime usage across the tests—particularly in the candles generator tests—it’s evident that the time calculations are consistent. The test constants are defined using
DateTime.utc(), and the helper (which calls something likeDateTime.fromSeconds(...).toUTC()) consistently returns a UTC DateTime. In addition, tests that mock time (using constructs such asjest.spyOn(DateTime, 'now')) appear to correctly reset or restore the mock (typically via afterEach or Jest’s auto–mock clearing) ensuring the system time is properly reinstated after each test.
- Test constants (e.g. in helpers/constants.ts) use
DateTime.utc(), ensuring a consistent UTC base.- Time arithmetic—including adding or subtracting minutes—is uniformly applied (with
.toISO()or.toUTC().toISO()), yielding equivalent UTC-formatted strings.- The mechanism for mocking the current time in tests adheres to Jest practices, with afterEach (or equivalent resets) restoring the original system time across tests.
Do not merge
Summary by CodeRabbit
PerpetualMarketCreateEventV3andUpdatePerpetualEventV3.defaultFundingRate1Hfor better funding rate management.oracle-price-candles.