-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IND-491]: Remove unused lastPrice from perpetual_markets table #813
Conversation
Warning Rate Limit Exceeded@Christopher-Li has exceeded the limit for the number of files or commits that can be reviewed per hour. Please wait 8 minutes and 37 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the @coderabbitai review command. Alternatively, push new commits to this PR. We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates involve the removal of the Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- indexer/services/comlink/public/swagger.json
Files selected for processing (15)
- indexer/packages/postgres/tests/helpers/constants.ts (3 hunks)
- indexer/packages/postgres/src/db/migrations/migration_files/20231128171435_remove_perpetual_market_last_price.ts (1 hunks)
- indexer/packages/postgres/src/models/perpetual-market-model.ts (4 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/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/websocket-documentation.md (3 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (1 hunks)
- indexer/services/comlink/src/types.ts (1 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (1 hunks)
- indexer/services/ender/src/handlers/perpetual-market-handler.ts (1 hunks)
- indexer/services/ender/src/scripts/dydx_perpetual_market_handler.sql (2 hunks)
- indexer/services/roundtable/src/lib/athena-ddl-tables/perpetual_markets.ts (2 hunks)
Files skipped from review due to trivial changes (11)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/src/models/perpetual-market-model.ts
- indexer/packages/postgres/src/types/db-model-types.ts
- indexer/packages/postgres/src/types/perpetual-market-types.ts
- indexer/packages/postgres/src/types/websocket-message-types.ts
- indexer/services/comlink/tests/lib/request-helpers/request-transformer.test.ts
- indexer/services/comlink/public/api-documentation.md
- indexer/services/comlink/public/websocket-documentation.md
- indexer/services/comlink/src/request-helpers/request-transformer.ts
- indexer/services/comlink/src/types.ts
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts
Additional comments: 4
indexer/services/ender/src/handlers/perpetual-market-handler.ts (1)
- 94-99: The removal of the
lastPrice
field from thegetPerpetualMarketCreateObject
method aligns with the overall goal of the pull request to remove the unusedlastPrice
from the codebase. Ensure that any external systems or services that rely on the output of this method are also updated to handle the absence of thelastPrice
field.indexer/services/ender/src/scripts/dydx_perpetual_market_handler.sql (2)
14-19: The changes in this hunk do not show the removal of the
lastPrice
field, but the summary indicates that it should be removed. Ensure that thelastPrice
field is indeed removed from theperpetual_market_record
and that any code handling this field is also updated accordingly.17-19: The fields
priceChange24H
,trades24H
, andvolume24H
are being set to 0. If these are placeholders for future implementation, consider adding a comment to clarify their purpose. If they are meant to be initialized to 0 and updated later, ensure that there is logic elsewhere in the code to update these values accordingly.indexer/services/roundtable/src/lib/athena-ddl-tables/perpetual_markets.ts (1)
- 31-33: The summary states that the
lastPrice
column is being removed, but the hunk shows a cast to double being added for other fields. Please verify that the changes in the hunk are consistent with the intended removal of thelastPrice
column and update the summary or the code accordingly.
return knex | ||
.schema | ||
.alterTable('perpetual_markets', (table) => { | ||
table.decimal('lastPrice', null).defaultTo('0').notNullable(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'down' migration function should specify the precision and scale for the decimal type of the 'lastPrice' column to ensure that it matches the original column specification before it was dropped.
- table.decimal('lastPrice', null).defaultTo('0').notNullable();
+ table.decimal('lastPrice', precision, scale).defaultTo('0').notNullable();
Replace precision
and scale
with the appropriate values that were used when the 'lastPrice' column was originally created.
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.
table.decimal('lastPrice', null).defaultTo('0').notNullable(); | |
table.decimal('lastPrice', precision, scale).defaultTo('0').notNullable(); |
4ea1070
to
4c5cd26
Compare
Changelist
Remove unused lastPrice from perpetual_markets table
Test Plan
Updated unit tests. While this is an indexer postgres breaking change, this should not cause any issues since lastPrice is not written to and is always '0' in all markets.
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.