[IND-494]: Create trading_rewards postgres table #822
Conversation
IND-494 Create trading_rewards postgres table
The Indexes to be added:
|
WalkthroughThe changes involve the introduction of a new Changes
TipsChat with CodeRabbit Bot (
|
![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: 5
Configuration used: CodeRabbit UI
Files selected for processing (11)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/stores/trading-rewards-table.test.ts (1 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20231129153017_create_trading_rewards_table.ts (1 hunks)
- indexer/packages/postgres/src/helpers/db-helpers.ts (1 hunks)
- indexer/packages/postgres/src/models/trading-reward-model.ts (1 hunks)
- indexer/packages/postgres/src/models/wallet-model.ts (3 hunks)
- indexer/packages/postgres/src/stores/trading-reward-table.ts (1 hunks)
- indexer/packages/postgres/src/types/db-model-types.ts (1 hunks)
- indexer/packages/postgres/src/types/index.ts (1 hunks)
- indexer/packages/postgres/src/types/query-types.ts (2 hunks)
- indexer/packages/postgres/src/types/trading-reward-types.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/src/helpers/db-helpers.ts
- indexer/packages/postgres/src/types/index.ts
Additional comments: 14
indexer/packages/postgres/src/db/migrations/migration_files/20231129153017_create_trading_rewards_table.ts (2)
14-14: Verify that the 'wallet' table exists and the 'address' column in the 'wallet' table is of type string to ensure the foreign key constraint is correctly established.
7-19: The migration script looks good, but ensure that the 'id' column is set to be a UUID type in the 'wallet' table as well, to maintain consistency with the foreign key relationship.
indexer/packages/postgres/src/models/trading-reward-model.ts (1)
- 42-42: The
blockHeightproperty is defined as a string with an integer pattern in the JSON schema. Verify if this is the intended type, as block heights are typically integers and not strings.indexer/packages/postgres/src/models/wallet-model.ts (3)
1-7: The import changes reflect the summary accurately, with the addition of
pathandModelfrom 'objection' andNonNegativeNumericPatternfrom '../lib/validators'.18-43: The relationMappings have been updated to include new relations and use
path.joinfor resolving model paths. Verify that the paths provided topath.joinare correct and that the corresponding model files exist.51-57: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [45-57]
The jsonSchema has been updated to use
NonNegativeNumericPatternfor thetotalTradingRewardsproperty. Ensure that theNonNegativeNumericPatternis correctly defined and enforces non-negative numeric values as intended.indexer/packages/postgres/src/stores/trading-reward-table.ts (4)
19-21: The
uuidfunction is marked with a TODO comment indicating that there is an intention to refactor the string concatenation to useArray.join. This should be tracked to ensure it is addressed in the future.24-79: The
findAllfunction correctly constructs a dynamic query based on the provided parameters and options. It's good to see that the function is using a base query setup and applying filters conditionally. This approach is maintainable and scalable.82-91: The
createfunction correctly inserts a new trading reward entry into the database using theuuidfunction to generate a unique ID. The use of the spread operator to include thetradingRewardToCreateobject is efficient and clean.94-104: The
findByIdfunction is implemented correctly, using thesetupBaseQueryhelper to initialize the query and thefindByIdmethod to retrieve the specific trading reward entry. The use of returning('*') ensures that all columns are returned.indexer/packages/postgres/src/types/db-model-types.ts (1)
- 226-232: The new
TradingRewardFromDatabaseinterface is correctly defined and consistent with the rest of the file and the provided summary.indexer/packages/postgres/src/types/query-types.ts (2)
74-74: The addition of
BLOCK_TIME_BEFORE_OR_ATto theQueryableFieldenum aligns with the summary provided.268-271: The
TradingRewardQueryConfiginterface has been correctly added, extendingQueryConfigwith the specified fields and types.indexer/packages/postgres/src/types/trading-reward-types.ts (1)
- 1-16: The changes to the
trading-reward-types.tsfile are correct and align with the summary provided. The import ofIsoString, the definition of theTradingRewardCreateObjectinterface, and theTradingRewardColumnsenum are all consistent with the intended use for the newtrading_rewardstable.
| table.string('address').notNullable(); | ||
| table.timestamp('blockTime').notNullable(); | ||
| table.bigInteger('blockHeight').notNullable(); | ||
| table.decimal('amount').notNullable(); |
There was a problem hiding this comment.
Consider specifying the precision and scale for the 'amount' column if the decimal values have a fixed format, to ensure data consistency and avoid potential issues with arithmetic operations on these values.
| it('Successfully creates a TradingReward', async () => { | ||
| await TradingRewardTable.create(defaultTradingReward); | ||
| }); |
There was a problem hiding this comment.
The test case 'Successfully creates a TradingReward' does not assert the result of the create operation. It is important to verify that the create method returns the expected result to ensure that the TradingReward was created successfully.
| it('Successfully finds a TradingReward', async () => { | ||
| await TradingRewardTable.create(defaultTradingReward); | ||
|
|
||
| const tradingReward: TradingRewardFromDatabase | undefined = await TradingRewardTable.findById( | ||
| TradingRewardTable.uuid(defaultTradingReward.address, defaultTradingReward.blockHeight), | ||
| ); | ||
|
|
||
| expect(tradingReward).toEqual(expect.objectContaining(defaultTradingReward)); |
There was a problem hiding this comment.
The test case 'Successfully finds a TradingReward' correctly asserts the expected result. However, it would be beneficial to also verify that the findById method returns undefined when a non-existent ID is passed. This would ensure that the method behaves correctly in scenarios where the TradingReward does not exist.
| * A mapping from column name to JSON conversion expected. | ||
| * See getSqlConversionForDydxModelTypes for valid conversions. | ||
| * | ||
| * TODO(IND-239): Ensure that jsonSchema() / sqlToJsonConversions() / model fields match. |
There was a problem hiding this comment.
The TODO comment indicates a need to ensure that jsonSchema(), sqlToJsonConversions(), and model fields match. If this has been addressed, the comment should be removed to avoid confusion.
| ], | ||
| properties: { | ||
| id: { type: 'string', format: 'uuid' }, | ||
| address: { type: 'string' }, |
There was a problem hiding this comment.
The address property in the JSON schema does not specify a format or pattern. Consider adding validation to ensure that addresses are stored in a consistent and valid format.
| baseQuery = baseQuery.orderBy( | ||
| TradingRewardColumns.blockHeight, | ||
| Ordering.ASC, | ||
| ); | ||
| } |
There was a problem hiding this comment.
nit: order by block time and address by default for a deterministic ordering.
There was a problem hiding this comment.
I did block height and address which should be the same
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/postgres/src/models/wallet-model.ts (3 hunks)
- indexer/packages/postgres/src/stores/trading-reward-table.ts (1 hunks)
Additional comments: 7
indexer/packages/postgres/src/models/wallet-model.ts (3)
1-1: The import of the
pathmodule is correctly added to support the new relation mapping in theWalletModel.38-38: The
jsonSchemadefinition fortotalTradingRewardscorrectly usesNonNegativeNumericPatternto ensure that the value is a non-negative number.18-26: The new relation mapping for
tradingRewardsis correctly defined, with themodelClassproperty usingpath.jointo resolve the path to thetrading-reward-model.indexer/packages/postgres/src/stores/trading-reward-table.ts (4)
34-42: The
verifyAllRequiredFieldsfunction is called with a constructed object andrequiredFields. Ensure that therequiredFieldsparameter is being used correctly and that it is necessary for the function's operation. IfrequiredFieldsare meant to be a subset of the fields inTradingRewardQueryConfig, then the typecasting toQueryConfigmight not be necessary.68-75: The default ordering is set to descending for both
blockHeightandaddress. Verify that this ordering is intentional and aligns with the expected behavior of thefindAllfunction. If not, consider adjusting the default ordering or providing a rationale for this choice.87-87: The default value for
optionsin thecreatefunction is{ txId: undefined }. Verify that this default is intentional and that it is safe to proceed with database operations without a transaction ID. If a transaction ID is required for certain operations, consider handling the case where it isundefined.97-107: The
findByIdfunction is being used to retrieve a trading reward by itsaddress. Typically,findByIdis used with a primary key, which is usually anidfield. Verify that theaddressfield is being used as a unique identifier for trading rewards and that there is no confusion with the primary key of the table.
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/src/models/wallet-model.ts (2 hunks)
Additional comments: 4
indexer/packages/postgres/src/models/wallet-model.ts (4)
1-7: The import statements have been updated correctly to reflect the changes in the summary.
26-31: The
jsonSchemafortotalTradingRewardshas been correctly updated to useNonNegativeNumericPatternfor validation.26-32: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [32-40]
Verify if
totalTradingRewardsrequires a conversion insqlToJsonConversionssimilar to theaddressfield, and if so, ensure it is added.
- 1-8: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [9-11]
The
relationMappingsobject is empty, but the summary indicates that there should be new relation mappings fortradingRewards. Ensure that the relation mappings are correctly implemented if they are part of this change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/packages/postgres/tests/stores/trading-rewards-table.test.ts (1 hunks)
- indexer/packages/postgres/src/models/wallet-model.ts (2 hunks)
Additional comments: 3
indexer/packages/postgres/src/models/wallet-model.ts (3)
1-1: The import statement has been correctly updated to reflect the use of
NonNegativeNumericPatternin the jsonSchema.22-27: The jsonSchema for 'totalTradingRewards' has been correctly updated to use
NonNegativeNumericPatternto ensure that the value is non-negative as intended.1-4: > Note: This review was outside the patches, and no patch overlapping with it was found. Original lines [8-11]
The summary indicates that there should be updates to relation mappings for
tradingRewards, but these are not present in the provided code. Please verify if the relation mappings have been implemented elsewhere or if they were mistakenly omitted.
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/src/db/migrations/migration_files/20231129153017_create_trading_rewards_table.ts (1 hunks)
Additional comments: 3
indexer/packages/postgres/src/db/migrations/migration_files/20231129153017_create_trading_rewards_table.ts (3)
11-11: Consider specifying the precision and scale for the 'amount' column if the decimal values have a fixed format, to ensure data consistency and avoid potential issues with arithmetic operations on these values.
14-14: Verify that the 'wallets' table and the 'address' column exist before setting up the foreign key relationship to avoid migration failures.
17-19: Ensure that the addition of indices on 'address', 'blockTime', and 'blockHeight' does not significantly impact write performance, especially if the table is expected to have a high write volume.
Changelist
Create trading_rewards postgres table
Test Plan
Test with 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.