[IND-499]: Add AggregateTradingRewardsProcessedCache with roundtable task #927
Conversation
WalkthroughThe codebase has undergone a set of updates to enhance its date handling and trading reward aggregation features. A new constant for representing a day in milliseconds was added, and utility functions for date rounding were introduced. The Postgres module now includes improved functionality for managing trading reward data, with new methods for retrieval and deletion based on specific criteria. Redis caching capabilities were expanded to handle aggregate trading rewards. Furthermore, the Roundtable service received configuration options for different aggregation intervals and updated task logic for trading reward aggregation. Changes
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's AI:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (15)
- indexer/packages/base/src/constants.ts (1 hunks)
- indexer/packages/base/src/date-helpers.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (3 hunks)
- indexer/packages/postgres/tests/stores/trading-reward-aggregation-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/trading-reward-aggregation-table.ts (3 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
- indexer/packages/redis/tests/caches/aggregate-trading-rewards-processed-cache.test.ts (1 hunks)
- indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/aggregate-trading-rewards.test.ts (2 hunks)
- indexer/services/roundtable/src/config.ts (2 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/lib/constants.ts (2 hunks)
- indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/base/src/index.ts
Additional comments: 34
indexer/services/roundtable/src/lib/constants.ts (2)
2-2: The import of
DateTimeOptionsfrom 'luxon' is correct and follows standard import conventions in TypeScript.14-14: The declaration of
UTC_OPTIONSis correct and follows TypeScript's conventions for declaring constants. It's also good practice to define such constants for date-time options to ensure consistency across the application.indexer/packages/base/src/date-helpers.ts (2)
5-10: The
floorDatefunction correctly calculates the floor of the given date to the nearest interval specified bytimeInMilliseconds. The logic usesMath.floorwhich is the appropriate choice for this operation.17-22: The
ceilingDatefunction correctly calculates the ceiling of the given date to the nearest interval specified bytimeInMilliseconds. The logic usesMath.ceilwhich is the appropriate choice for this operation.indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts (3)
8-10: The
getKeyfunction correctly constructs the Redis key using theAGGREGATE_TRADING_REWARDS_PROCESSED_CACHE_KEYprefix and the given period. This ensures that keys are unique per period.12-16: The
getProcessedTimefunction correctly retrieves the processed time from Redis. It uses thegetAsynchelper, which is expected to be a promisified version of Redis's get command.19-24: The
setProcessedTimefunction correctly sets the processed time in Redis. The use ofawaitensures that the operation completes before the function resolves, which is important for ensuring data consistency.indexer/packages/base/src/constants.ts (1)
- 15-15: The addition of
ONE_DAY_IN_MILLISECONDSis correct and follows the established pattern of defining time-related constants in milliseconds. This constant will be useful for any operations that need to represent a day in terms of milliseconds.indexer/packages/redis/src/index.ts (1)
- 3-3: The export of
AggregateTradingRewardsProcessedCacheis correct and follows the established pattern of exporting modules in TypeScript. This will make the cache available for import in other parts of the application.indexer/packages/redis/__tests__/caches/aggregate-trading-rewards-processed-cache.test.ts (1)
- 9-37: The test case 'successfully sets and gets aggregate trading rewards processed' is well-structured. It checks for the absence of a value before setting it and then confirms that the value can be retrieved correctly. This is a good practice for testing cache operations.
indexer/packages/postgres/__tests__/helpers/mock-generators.ts (2)
8-8: The import of
WalletTableis correct and follows the established pattern of importing modules in TypeScript. This will allow the use ofWalletTablefor creating mock wallet data in tests.67-67: The addition of
defaultWalletto theseedDatafunction is correct. It ensures that the wallet data is available for tests that depend on it, which is important for setting up a consistent test environment.indexer/services/roundtable/src/index.ts (1)
- 126-147: The conditional checks for
LOOPS_ENABLED_AGGREGATE_TRADING_REWARDS_DAILY,LOOPS_ENABLED_AGGREGATE_TRADING_REWARDS_WEEKLY, andLOOPS_ENABLED_AGGREGATE_TRADING_REWARDS_MONTHLYare correct. They follow the established pattern for starting loops based on configuration flags. The use ofTradingRewardAggregationPeriodenum values as arguments toaggregateTradingRewardsTasksis also correct, ensuring that the tasks are started with the appropriate period.indexer/packages/postgres/src/stores/trading-reward-aggregation-table.ts (2)
95-108: The
getLatestAggregatedTradeRewardfunction is correctly implemented to retrieve the latest aggregated trade reward for a given period. The use oforderBywithOrdering.DESCandfirst()ensures that the most recent record is returned.156-177: The
deleteAllfunction is correctly implemented to delete aggregated trade rewards based on the period and starting height. The use of a conditional query builder pattern allows for flexible query construction based on the provided parameters.indexer/services/roundtable/src/config.ts (2)
45-47: The addition of configuration options for enabling aggregate trading rewards on different intervals (daily, weekly, monthly) is correct. The default values are set to
true, which is a reasonable default for feature flags.80-87: The loop interval configurations for the aggregate trading rewards have been correctly added and follow the established pattern for loop timing configurations. The default values are consistent with other loop intervals in the system.
indexer/packages/postgres/__tests__/stores/trading-reward-aggregation-table.test.ts (3)
1-11: The addition of new imports for
TradingRewardAggregationPeriod,createdDateTime, andBlockTableindicates that the test cases will be using these entities. Ensure that the new test cases are using these imports correctly and that they are necessary for the test logic.63-94: The test case for finding the latest monthly
TradingRewardAggregationseems to correctly create blocks and trading reward aggregations before attempting to retrieve the latest one. It's important to ensure that theTradingRewardAggregationTable.getLatestAggregatedTradeRewardmethod is designed to return the correct record based on theperiodparameter.139-177: The test case for deleting trading reward aggregations after a certain height correctly sets up the necessary data and then uses the
deleteAllmethod with thestartedAtHeightOrAfterparameter. It's crucial to verify that thedeleteAllmethod is implemented to handle this parameter correctly and that the test asserts the expected state of the database after the deletion.indexer/packages/postgres/src/types/query-types.ts (2)
75-79: The addition of
STARTED_AT_HEIGHT_OR_AFTERto theQueryableFieldenum is consistent with the changes in the test file that uses this field. Ensure that this new field is supported in the query logic whereTradingRewardAggregationQueryConfigis used.280-285: The
TradingRewardAggregationQueryConfiginterface has been updated to include the newSTARTED_AT_HEIGHT_OR_AFTERfield. This change should be cross-checked with the implementation of the query logic to ensure that it correctly interprets and uses this new field.indexer/services/roundtable/__tests__/tasks/aggregate-trading-rewards.test.ts (12)
2-15: The new imports and constants are introduced to support the new caching mechanism and interval generation. Ensure that these imports are used within the file and that the constants are correctly implemented in the tests.
39-57: The constants for date and time calculations are correctly using the
luxonlibrary to ensure UTC time is used. This is important for consistency in time-based calculations across different time zones.59-79: The test case for
getTradingRewardDataToProcessIntervalcorrectly checks for undefined return when no blocks are in the database. The use oflogger.infoto log this condition is also appropriate.81-102: The test case for
getTradingRewardDataToProcessIntervalwhen the cache is empty and no aggregations are present correctly asserts the interval based on the first block time. This ensures that the interval calculation logic is working as expected.104-134: The test case for deleting incomplete aggregations when the cache is empty is important to ensure that the system can recover from an inconsistent state. The assertions for the interval and the remaining aggregations are correctly checking the expected outcomes.
138-163: The test case for handling multiple incomplete aggregations when the cache is empty is similar to the previous one but with more data. It's good to see that the test is comprehensive and covers different scenarios.
166-196: The test case for handling intervals when the cache is populated but not enough blocks have been processed is crucial for ensuring that the system correctly identifies the next interval to process. The assertions are well-placed to check the expected behavior.
199-229: The test case for handling intervals when the cache is populated and there are unprocessed blocks beyond a certain time threshold is another important scenario. The test ensures that the system can correctly calculate the next interval to process.
233-262: The test case for handling intervals when there is a significant amount of unprocessed blocks is testing the system's ability to handle larger intervals. This is a good stress test for the interval calculation logic.
266-298: The test case for handling intervals when the cache is populated close to the end of the day is testing edge cases around day boundaries. This is important for ensuring that the system behaves correctly around these critical times.
302-312: The test case for
runTaskwhen there are no blocks in the database is ensuring that the task exits gracefully. The use oflogger.infoto log this condition is appropriate and should aid in debugging if needed.316-321: The helper function
createBlockWithTimeis a good addition to the test suite, allowing for the creation of blocks with specific times for testing interval calculations. Ensure that this function is used correctly in the test cases.
| TradingRewardAggregationFromDatabase | undefined = await | ||
| TradingRewardAggregationTable.getLatestAggregatedTradeReward(this.period); | ||
|
|
||
| if (latestAggregation !== undefined && latestAggregation.endedAt === null) { | ||
| await TradingRewardAggregationTable.deleteAll({ | ||
| period: this.period, | ||
| startedAtHeightOrAfter: latestAggregation.startedAtHeight, | ||
| }); | ||
| logger.info({ | ||
| at: 'aggregate-trading-rewards#deleteIncompleteAggregatedTradingReward', | ||
| message: `Deleted the last ${this.period} aggregated trading rewards`, | ||
| height: latestAggregation.startedAtHeight, | ||
| time: latestAggregation.startedAt, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * The start time of the next interval to process. This will be the start time of the | ||
| * first block in the database, or the start time of the next month after the latest | ||
| * monthly aggregation. | ||
| */ | ||
| private async getNextIntervalStart(): Promise<DateTime> { | ||
| const latestAggregation: | ||
| TradingRewardAggregationFromDatabase | undefined = await | ||
| TradingRewardAggregationTable.getLatestAggregatedTradeReward(this.period); | ||
| // Since we've deleted the incomplete aggregations, we can assume that the latestAggregation | ||
| // is complete. | ||
| if (latestAggregation !== undefined) { | ||
| return DateTime.fromISO(latestAggregation.endedAt!, UTC_OPTIONS); | ||
| } | ||
|
|
||
| // Since we were able to find the latest block, we assume we can find the first block | ||
| const firstBlock: BlockFromDatabase[] = await BlockTable.findAll({ | ||
| blockHeight: ['1'], | ||
| limit: 1, | ||
| }, []); | ||
| return DateTime.fromISO(firstBlock[0].time, UTC_OPTIONS); | ||
| } | ||
|
|
||
| /** | ||
| * Generate the interval that will be processed. The end time of the interval is calculated from | ||
| * a start time and the latest block. This will be the earliest of the following: | ||
| * 1. The next day | ||
| * 2. An hour after start time | ||
| * 3. The start of the minute of the latest block | ||
| * @param startTime - startTime of the interval | ||
| * @param latestBlock | ||
| * @returns | ||
| */ | ||
| private generateInterval( | ||
| startTime: DateTime, | ||
| latestBlock: BlockFromDatabase, | ||
| ): Interval { | ||
| const latestBlockTime: Date = DateTime.fromISO(latestBlock.time, UTC_OPTIONS).toJSDate(); | ||
|
|
||
| // The most recent start of a minute. i.e 12:02:33 will be rounded to 12:02:00 | ||
| const normalizedLatestBlockTime: Date = floorDate( | ||
| latestBlockTime, | ||
| ONE_MINUTE_IN_MILLISECONDS, | ||
| ); | ||
|
|
||
| const nextDay: Date = startTime.plus({ days: 1 }).toJSDate(); | ||
| const normalizedNextDay: Date = floorDate(nextDay, ONE_DAY_IN_MILLISECONDS); | ||
|
|
||
| const startDate: Date = startTime.toJSDate(); | ||
| const oneHourAfterStart: Date = DateTime.fromJSDate(startDate).plus({ hour: 1 }).toJSDate(); | ||
| const endTime: Date = new Date(Math.min( | ||
| normalizedLatestBlockTime.getTime(), | ||
| normalizedNextDay.getTime(), | ||
| oneHourAfterStart.getTime(), | ||
| )); | ||
| const endDateTime: DateTime = DateTime.fromJSDate(endTime).toUTC(); | ||
| return Interval.fromDateTimes(startTime, endDateTime); | ||
| } | ||
|
|
||
| private async getTradingRewardDataToProcess( | ||
| _interval: Interval, | ||
| ): Promise<TradingRewardFromDatabase[]> { | ||
| // TODO: Implement | ||
| return Promise.resolve([]); | ||
| } | ||
|
|
||
| private sortTradingRewardData( | ||
| _tradingRewardData: TradingRewardFromDatabase[], | ||
| ): SortedTradingRewardData { | ||
| // TODO: Implement | ||
| return {}; | ||
| } | ||
|
|
||
| async function updateTradingRewardsAggregation( | ||
| _sortedTradingRewardData: SortedTradingRewardData, | ||
| ): Promise<void> { | ||
| // TODO: Implement | ||
| private async updateTradingRewardsAggregation( | ||
| _sortedTradingRewardData: SortedTradingRewardData, | ||
| ): Promise<void> { | ||
| // TODO: Implement | ||
| } | ||
| } |
There was a problem hiding this comment.
The new AggregateTradingReward class and generateTaskFromPeriod function have been added to the aggregate-trading-rewards.ts file. It's important to ensure that the class methods are correctly implemented and that the generateTaskFromPeriod function correctly instantiates and uses the AggregateTradingReward class. Additionally, the TODO comments within the class methods indicate that the implementation is not yet complete, which should be addressed before merging.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/postgres/tests/stores/wallet-table.test.ts (3 hunks)
Additional comments: 6
indexer/packages/postgres/__tests__/stores/wallet-table.test.ts (6)
3-3: Renaming
defaultWallettodefaultWallet2may indicate the use of multiple wallet constants for testing. Verify thatdefaultWallet2is correctly defined and used consistently throughout the test cases.20-20: Ensure that the
createmethod is intended to be called withdefaultWallet2and that this change is consistent with the test's purpose.24-35: The
upsertmethod is called twice withdefaultWallet2. Confirm that the test logic correctly reflects the intention of upserting the same wallet data multiple times and that the assertions are valid.Verification successful
To verify the structure of
defaultWallet2and the behavior of theupsertmethod, I will generate a script to extract the definition ofdefaultWallet2and theupsertmethod from thewallet-table.test.tsfile.
I will generate a new script to extract the full definition of the
upsertmethod and to find the definition ofdefaultWallet2in thewallet-table.test.tsfile.
The
upsertfunction inwallet-table.tstakes awalletToUpsertobject of typeWalletCreateObjectand anoptionsobject with an optionaltxId. It returns a promise that resolves to aWalletFromDatabaseobject. The function performs an upsert operation and returns the first wallet from the result, which is expected to be the only wallet due to the nature of upsert operations.The
defaultWallet2variable is imported from../helpers/constantsand used in various test cases withinwallet-table.test.ts, including calls tocreateandupsertmethods onWalletTable, and it is expected to match the structure required by theupsertmethod.Given this information, the test logic appears to be intentionally upserting the same wallet data multiple times to confirm that the
upsertoperation behaves correctly when inserting a new wallet and when updating an existing wallet. The assertions usingexpect.objectContaining(defaultWallet2)are checking that the wallet returned from the database matches the data that was upserted.Based on the provided context and the new information from the scripts, the review comment questioning the intention of upserting the same wallet data multiple times and the validity of the assertions seems to be addressed. The test is designed to verify the idempotency of the
upsertoperation, which is a common practice in testing database operations.* 40-43: The creation of a wallet with a hardcoded 'fake_address' should be reviewed to ensure it aligns with the test's objectives and that the address is not used elsewhere in a conflicting manner.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the upsert method is compatible with defaultWallet2 structure and is intended to be called multiple times. ast-grep --lang typescript --pattern $'WalletTable.upsert($_)'Length of output: 293
Script:
#!/bin/bash # Extract the definition of defaultWallet2 and the upsert method from the wallet-table.test.ts file. rg "let defaultWallet2 =" ./indexer/packages/postgres/__tests__/stores/wallet-table.test.ts rg "upsert" ./indexer/packages/postgres/src/stores/wallet-table.tsLength of output: 233
Script:
#!/bin/bash # Extract the full definition of the upsert method from the wallet-table.ts file. rg -A 10 "export async function upsert" ./indexer/packages/postgres/src/stores/wallet-table.ts # Attempt to find the definition of defaultWallet2 in the wallet-table.test.ts file. rg "defaultWallet2" ./indexer/packages/postgres/__tests__/stores/wallet-table.test.tsLength of output: 1129
54-57: The assertion checks if the wallet object contains properties of
defaultWallet2. Confirm that the test is correctly asserting the expected properties and thatdefaultWallet2is the intended object for comparison.61-67: The
createandfindByIdmethods are used in conjunction withdefaultWallet2. Verify that thefindByIdmethod is expected to retrieve the wallet created withdefaultWallet2and that the assertion is correct.Verification successful
The search results indicate that both
defaultWallet2anddefaultAddressare imported from../helpers/constantsin thewallet-table.test.tsfile. ThedefaultWallet2object is used in thecreate,upsert, andexpectcalls, anddefaultAddressis used as an argument to thefindByIdmethod. Given that they are imported from the same file, it is reasonable to infer thatdefaultAddressis related todefaultWallet2, likely representing the address of the wallet.Since
defaultWallet2is used to create and upsert a wallet, anddefaultAddressis used to find a wallet, the test expects that the wallet found byfindByIdusingdefaultAddresswill contain the data fromdefaultWallet2. The assertionexpect(wallet).toEqual(expect.objectContaining(defaultWallet2))is checking that the wallet returned byfindByIdmatches the expected fields ofdefaultWallet2.Based on this information, it can be concluded that the
findByIdmethod is expected to retrieve the wallet created withdefaultWallet2, and the assertion is correctly checking this.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that findById is expected to work with the wallet created with defaultWallet2 and that the assertion is correct. ast-grep --lang typescript --pattern $'WalletTable.findById($_)'Length of output: 2707
Script:
#!/bin/bash # Verify the relationship between defaultWallet2, defaultAddress, and the findById method. # Also, check the assertion for correctness. rg 'let defaultWallet2 =' ./indexer/packages/postgres/__tests__/stores/wallet-table.test.ts rg 'const defaultAddress =' ./indexer/packages/postgres/__tests__/stores/wallet-table.test.tsLength of output: 189
Script:
#!/bin/bash # Broaden the search for defaultWallet2 and defaultAddress without assuming specific variable declaration keywords. # Also, search for any import statements that might include these variables. rg 'defaultWallet2' ./indexer/packages/postgres/__tests__/stores/wallet-table.test.ts rg 'defaultAddress' ./indexer/packages/postgres/__tests__/stores/wallet-table.test.tsLength of output: 885
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/postgres/tests/stores/trading-reward-aggregation-table.test.ts (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/postgres/tests/stores/trading-reward-aggregation-table.test.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/packages/postgres/tests/stores/wallet-table.test.ts (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- indexer/packages/postgres/tests/stores/wallet-table.test.ts
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- indexer/packages/postgres/src/stores/wallet-table.ts (3 hunks)
- indexer/packages/postgres/src/types/wallet-types.ts (1 hunks)
- indexer/services/ender/tests/handlers/trading-rewards-handler.test.ts (1 hunks)
Additional comments: 4
indexer/packages/postgres/src/types/wallet-types.ts (1)
- 8-11: The addition of
WalletUpdateObjectaligns with the existing structure and naming conventions of the wallet types.indexer/packages/postgres/src/stores/wallet-table.ts (2)
16-16: The import of
WalletUpdateObjectis correctly added for use in the newupdatefunction.74-87: Verify the return type of the
patchmethod to ensure that the cast to a singleWalletFromDatabaseobject is correct.Verification successful
The previous script contained an error due to the incorrect usage of the
--execflag withrg. Let's correct the script to search for thepatchmethod within TypeScript files and examine the output to verify the return type of thepatchmethod.
It seems there was an issue with the file type specification in the
rgcommand. Ripgrep may not recognize "typescript" as a file type directly. Instead, we can search for files with the.tsextension. Let's adjust the script accordingly to search for thepatchmethod in.tsfiles.
The output from the script shows several instances of the
patchmethod being used in the context of the Objection.js ORM within theindexer/packages/postgres/src/stores/directory. In each case, the method is chained with.returning('*'), which suggests that the expected behavior is to return the updated row(s) from the database.Given this pattern, it's reasonable to infer that the
patchmethod followed by.returning('*')is intended to return the updated object, and the cast to a singleWalletFromDatabaseobject in thewallet-table.tsfile is likely correct. However, without explicit documentation or type definitions confirming this behavior, we cannot be entirely certain.Based on the observed usage pattern in the codebase, the review comment regarding the return type of the
patchmethod and the cast to a single object seems to be consistent with the other usages in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the return type of the patch method in the Objection.js model rg --type typescript --files-with-matches 'patch' --exec cat {} +Length of output: 476
Script:
#!/bin/bash # Search for the usage of the patch method in TypeScript files to infer the return type rg --type typescript 'patch'Length of output: 63
Script:
#!/bin/bash # Search for the usage of the patch method in .ts files to infer the return type rg 'patch' --glob '*.ts'Length of output: 2972
indexer/services/ender/__tests__/handlers/trading-rewards-handler.test.ts (1)
- 197-200: The test case has been correctly updated to use the
updatemethod fromWalletTable.
| @@ -1,6 +1,6 @@ | |||
| import { WalletFromDatabase } from '../../src/types'; | |||
| import { clearData, migrate, teardown } from '../../src/helpers/db-helpers'; | |||
| import { defaultAddress, defaultWallet } from '../helpers/constants'; | |||
| import { defaultWallet2 } from '../helpers/constants'; | |||
There was a problem hiding this comment.
Since defaultWallet is created by default now, and half of the functions in wallet-table are testing behavior before and after the wallet is created, it was easiest to just use defaultWallet2 instead of trying to customize the logic
| @@ -194,7 +194,7 @@ describe('tradingRewardHandler', () => { | |||
| txHash: defaultTxHash, | |||
| }); | |||
|
|
|||
| await WalletTable.create({ | |||
| await WalletTable.update({ | |||
There was a problem hiding this comment.
yes, because the default walletAddress already exists, in seed, so we are changing this operation from create to update
indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Outdated
Show resolved
Hide resolved
indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Outdated
Show resolved
Hide resolved
indexer/packages/redis/__tests__/caches/aggregate-trading-rewards-processed-cache.test.ts
Outdated
Show resolved
Hide resolved
| @@ -75,8 +77,14 @@ export const configSchema = { | |||
| LOOPS_INTERVAL_MS_REMOVE_OLD_ORDER_UPDATES: parseInteger({ | |||
| default: THIRTY_SECONDS_IN_MILLISECONDS, | |||
| }), | |||
| LOOPS_INTERVAL_MS_AGGREGATE_TRADING_REWARDS: parseInteger({ | |||
| default: ONE_MINUTE_IN_MILLISECONDS, | |||
| LOOPS_INTERVAL_MS_AGGREGATE_TRADING_REWARDS_DAILY: parseInteger({ | |||
There was a problem hiding this comment.
do we need separate configs for each period? is there a case where they'll be individually turned on/off?
There was a problem hiding this comment.
combine these together
indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Outdated
Show resolved
Hide resolved
| redisClient, | ||
| ); | ||
| if (processedTime === null) { | ||
| await this.deleteIncompleteAggregatedTradingReward(); |
There was a problem hiding this comment.
It's a little confusing that this getTradingRewardDataToProcessInterval function has the side effect of deleting trading reward aggregation. Up to you whether you want to refactor this part out.
There was a problem hiding this comment.
moved it out, good call out
indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Outdated
Show resolved
Hide resolved
indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Outdated
Show resolved
Hide resolved
indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/src/models/trading-reward-aggregation-model.ts (1 hunks)
- indexer/packages/redis/tests/caches/aggregate-trading-rewards-processed-cache.test.ts (1 hunks)
- indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/aggregate-trading-rewards.test.ts (2 hunks)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts (1 hunks)
Additional comments: 8
indexer/packages/redis/__tests__/caches/aggregate-trading-rewards-processed-cache.test.ts (1)
- 1-38: The test case for setting and getting processed times in Redis is well-structured and covers the expected functionality.
indexer/packages/postgres/src/models/trading-reward-aggregation-model.ts (1)
- 54-54: The update to the comment for the
endedAtproperty in the jsonSchema correctly reflects the change in period end time logic from inclusive to exclusive.indexer/services/roundtable/src/index.ts (1)
- 126-147: The conditional blocks for starting the aggregate trading rewards loops are correctly structured and use the appropriate parameters for daily, weekly, and monthly aggregation periods.
indexer/services/roundtable/src/config.ts (3)
45-47: The new configuration options for enabling aggregate trading rewards on different bases are correctly added and set to true by default.
81-81: The modification of the loop interval value for aggregate trading rewards to
THIRTY_SECONDS_IN_MILLISECONDSis correctly implemented.138-138: The addition of
AGGREGATE_TRADING_REWARDS_MAX_INTERVAL_SIZE_MSwith a default value ofONE_HOUR_IN_MILLISECONDSis correctly implemented.indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts (1)
- 33-39: The
generateTaskFromPeriodfunction is correctly structured to return a task function based on the provided period.indexer/services/roundtable/__tests__/tasks/aggregate-trading-rewards.test.ts (1)
- 40-341: The test cases for the
AggregateTradingRewardclass methods are comprehensive and cover various scenarios, including cache states and block data presence.
| import { IsoString, TradingRewardAggregationPeriod } from '@dydxprotocol-indexer/postgres'; | ||
| import { RedisClient } from 'redis'; | ||
|
|
||
| import { getAsync } from '../helpers/redis'; | ||
|
|
||
| /** | ||
| * Cache key for the aggregate trading rewards processed cache. Given a | ||
| * TradingRewardAggregationPeriod, this cache stores the timestamp of the | ||
| * trading rewards that have been processed up to and excluding that timestamp. | ||
| */ | ||
| export const AGGREGATE_TRADING_REWARDS_PROCESSED_CACHE_KEY: string = 'v4/aggregate_trading_rewards_processed/'; | ||
|
|
||
| function getKey(period: TradingRewardAggregationPeriod): string { | ||
| return `${AGGREGATE_TRADING_REWARDS_PROCESSED_CACHE_KEY}${period}`; | ||
| } | ||
|
|
||
| export async function getProcessedTime( | ||
| period: TradingRewardAggregationPeriod, | ||
| client: RedisClient, | ||
| ): Promise<IsoString | null> { | ||
| return getAsync(getKey(period), client); | ||
| } | ||
|
|
||
| export async function setProcessedTime( | ||
| period: TradingRewardAggregationPeriod, | ||
| timestamp: IsoString, | ||
| client: RedisClient, | ||
| ): Promise<void> { | ||
| await client.set(getKey(period), timestamp); | ||
| } |
There was a problem hiding this comment.
Please add a file-level comment explaining how the AggregateTradingRewardsProcessedCache is utilized within the system for clarity.
| export class AggregateTradingReward { | ||
| period: TradingRewardAggregationPeriod; | ||
|
|
||
| // TODO(IND-499): Setup AggregateTradingRewardsProcessedCache for start time and add end time | ||
| return { | ||
| start: DateTime.fromISO(latestBlock.time), | ||
| end: DateTime.fromISO(latestBlock.time), | ||
| }; | ||
| } | ||
| constructor(period: TradingRewardAggregationPeriod) { | ||
| this.period = period; | ||
| } | ||
|
|
||
| async function getTradingRewardDataToProcess( | ||
| _interval: Interval, | ||
| ): Promise<TradingRewardFromDatabase[]> { | ||
| // TODO: Implement | ||
| return Promise.resolve([]); | ||
| } | ||
| async runTask(): Promise<void> { | ||
| await this.maybeDeleteIncompleteAggregatedTradingReward(); | ||
| const interval: Interval = await this.getTradingRewardDataToProcessInterval(); | ||
| logger.info({ | ||
| at: 'aggregate-trading-rewards#runTask', | ||
| message: 'Generated interval to aggregate trading rewards', | ||
| start: interval.start.toISO(), | ||
| end: interval.end.toISO(), | ||
| }); | ||
|
|
||
| function sortTradingRewardData( | ||
| _tradingRewardData: TradingRewardFromDatabase[], | ||
| ): SortedTradingRewardData { | ||
| // TODO: Implement | ||
| return {}; | ||
| } | ||
| const tradingRewardData: | ||
| TradingRewardFromDatabase[] = await this.getTradingRewardDataToProcess(interval); | ||
| const sortedTradingRewardData: SortedTradingRewardData = this.sortTradingRewardData( | ||
| tradingRewardData, | ||
| ); | ||
| await this.updateTradingRewardsAggregation(sortedTradingRewardData); | ||
| await AggregateTradingRewardsProcessedCache.setProcessedTime( | ||
| this.period, | ||
| interval.end.toISO(), | ||
| redisClient, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The AggregateTradingReward class is well-structured with a clear constructor and method for running the task. Ensure that the TODOs for implementing the data processing logic are addressed.
Christopher-Li
force-pushed
the
cl_cache
branch
from
56608e9
to
522a238
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (19)
- indexer/packages/base/src/date-helpers.ts (1 hunks)
- indexer/packages/base/src/index.ts (1 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (3 hunks)
- indexer/packages/postgres/tests/stores/trading-reward-aggregation-table.test.ts (4 hunks)
- indexer/packages/postgres/tests/stores/wallet-table.test.ts (3 hunks)
- indexer/packages/postgres/src/models/trading-reward-aggregation-model.ts (1 hunks)
- indexer/packages/postgres/src/stores/trading-reward-aggregation-table.ts (3 hunks)
- indexer/packages/postgres/src/stores/wallet-table.ts (3 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
- indexer/packages/postgres/src/types/wallet-types.ts (1 hunks)
- indexer/packages/redis/tests/caches/aggregate-trading-rewards-processed-cache.test.ts (1 hunks)
- indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts (1 hunks)
- indexer/packages/redis/src/index.ts (1 hunks)
- indexer/services/ender/tests/handlers/trading-rewards-handler.test.ts (1 hunks)
- indexer/services/roundtable/tests/tasks/aggregate-trading-rewards.test.ts (2 hunks)
- indexer/services/roundtable/src/config.ts (3 hunks)
- indexer/services/roundtable/src/index.ts (2 hunks)
- indexer/services/roundtable/src/lib/constants.ts (2 hunks)
- indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts (1 hunks)
Files skipped from review as they are similar to previous changes (7)
- indexer/packages/postgres/src/models/trading-reward-aggregation-model.ts
- indexer/packages/redis/tests/caches/aggregate-trading-rewards-processed-cache.test.ts
- indexer/packages/redis/src/caches/aggregate-trading-rewards-processed-cache.ts
- indexer/services/roundtable/tests/tasks/aggregate-trading-rewards.test.ts
- indexer/services/roundtable/src/config.ts
- indexer/services/roundtable/src/index.ts
- indexer/services/roundtable/src/tasks/aggregate-trading-rewards.ts
Additional comments: 22
indexer/packages/postgres/src/types/wallet-types.ts (1)
- 8-11: The addition of the
WalletUpdateObjectinterface is consistent with the PR's objectives to enhance the trading rewards system, specifically by allowing updates to wallets with trading rewards.indexer/services/roundtable/src/lib/constants.ts (2)
2-2: The import of
DateTimeOptionsfrom 'luxon' is appropriate for handling date and time operations, which is likely relevant to the new caching and task scheduling features.14-14: The declaration of
UTC_OPTIONSwith{ zone: 'utc' }is a good practice for ensuring consistent time zone handling across the application.indexer/packages/base/src/index.ts (1)
- 15-15: Exporting the
date-helpersmodule aligns with the PR's objectives to introduce date-related utility functions, which are likely used in the new caching mechanism and task enhancements.indexer/packages/base/src/date-helpers.ts (2)
5-10: The
floorDatefunction correctly implements the flooring of a date to the nearest interval defined bytimeInMilliseconds, which is useful for rounding dates in the context of reward processing.17-22: Similarly, the
ceilingDatefunction correctly implements the ceiling of a date to the nearest interval, complementing thefloorDatefunction and providing necessary functionality for date rounding.indexer/packages/redis/src/index.ts (1)
- 3-3: The export of
AggregateTradingRewardsProcessedCacheis in line with the PR's objectives to introduce a new caching mechanism for aggregate trading rewards.indexer/packages/postgres/__tests__/stores/wallet-table.test.ts (4)
20-20: The test case for creating a wallet is updated to use
defaultWallet2instead ofdefaultWallet, which is consistent with the explanation provided in the previous comments.24-33: The test case for upserting a wallet multiple times and validating the wallet's state after each upsert operation is correctly updated to use
defaultWallet2.52-55: The test case for finding all wallets and asserting their properties is correctly updated to use
defaultWallet2and the newfake_address.59-65: The test case for finding a specific wallet by address is correctly updated to use
defaultWallet2.indexer/packages/postgres/__tests__/helpers/mock-generators.ts (2)
8-8: The addition of
WalletTableto the import statements is necessary for creating adefaultWalletin theseedDatafunction, which is consistent with the changes in the test files.67-67: The creation of a
defaultWalletin theseedDatafunction is necessary for setting up the test environment, which now includes wallet-related operations.indexer/packages/postgres/src/stores/wallet-table.ts (2)
1-1: The addition of
PartialModelObjectto the import statement is necessary for the newupdatefunction, which allows partial updates to wallet records.74-86: The new
updatefunction is correctly implemented to handle wallet updates, using theWalletUpdateObjectandPartialModelObjectto perform a patch operation.indexer/packages/postgres/src/stores/trading-reward-aggregation-table.ts (2)
95-109: The addition of the
getLatestAggregatedTradeRewardfunction is consistent with the PR's objectives to retrieve the latest aggregated trade reward, which is likely used in the new task enhancements.156-177: The new
deleteAllfunction is correctly implemented to delete aggregated trade rewards based on specified criteria, which is necessary for maintaining the integrity of the reward aggregation system.indexer/packages/postgres/__tests__/stores/trading-reward-aggregation-table.test.ts (2)
61-91: The test case for finding the latest monthly TradingRewardAggregation is correctly implemented, ensuring that the most recent aggregation is retrieved as expected.
137-174: The test case for deleting trading reward aggregations after a certain height is correctly implemented, ensuring that the
deleteAllfunction works as intended.indexer/services/ender/__tests__/handlers/trading-rewards-handler.test.ts (1)
- 197-200: The test logic is altered to update an existing entry in the
WalletTableinstead of creating a new one, which is consistent with the changes in the wallet handling logic.indexer/packages/postgres/src/types/query-types.ts (2)
78-78: The addition of
STARTED_AT_HEIGHT_OR_AFTERto theQueryableFieldenum is necessary for querying trading reward aggregations based on a new criterion.283-283: The update to the
TradingRewardAggregationQueryConfiginterface to include theSTARTED_AT_HEIGHT_OR_AFTERfield is consistent with the new querying capabilities introduced in the PR.
Changelist
Test Plan
Extensive unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.