-
Notifications
You must be signed in to change notification settings - Fork 124
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-486] add new trade type #789
Conversation
The provided walkthrough and changes are already well-structured and aligned with the instructions. Therefore, no further modifications are necessary. 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: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- indexer/services/comlink/public/swagger.json
Files selected for processing (11)
- indexer/packages/postgres/src/types/index.ts (1 hunks)
- indexer/packages/postgres/src/types/trade-types.ts (1 hunks)
- indexer/packages/postgres/src/types/websocket-message-types.ts (2 hunks)
- indexer/services/comlink/public/api-documentation.md (2 hunks)
- indexer/services/comlink/public/websocket-documentation.md (3 hunks)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (2 hunks)
- indexer/services/comlink/src/types.ts (2 hunks)
- indexer/services/ender/tests/helpers/constants.ts (2 hunks)
- indexer/services/ender/tests/helpers/indexer-proto-helpers.ts (2 hunks)
- indexer/services/ender/tests/lib/kafka-publisher.test.ts (4 hunks)
- indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (2 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/src/types/index.ts
- indexer/packages/postgres/src/types/websocket-message-types.ts
Additional comments: 20
indexer/services/ender/src/handlers/order-fills/abstract-order-fill-handler.ts (1)
- 427-431: The
fillTypeToTradeType
function is used to convertFillType
toTradeType
. Ensure that this function handles all possibleFillType
values correctly and that theTradeType
enum includes all necessary values.indexer/packages/postgres/src/types/trade-types.ts (2)
1-10: The new
TradeType
enum is well defined and the comments provide clear explanations for each type.12-23: The
fillTypeToTradeType
function is correctly implemented. It handles all possibleFillType
values and throws an error for unknown types. However, it's important to ensure that this error is properly handled wherever this function is used to prevent unexpected crashes.indexer/services/ender/__tests__/helpers/indexer-proto-helpers.ts (2)
29-33: The new import
fillTypeToTradeType
is correctly imported from@dydxprotocol-indexer/postgres
.806-812: The function
fillTypeToTradeType
is correctly used to convert thetakerFill.type
to a trade type. Ensure that the functionfillTypeToTradeType
correctly handles all possible values oftakerFill.type
.indexer/services/ender/__tests__/lib/kafka-publisher.test.ts (4)
14-19: The import of
TradeType
is correct and its usage in the codebase should be verified to ensure that it replacesFillType
as intended.410-414: The
TradeType.LIMIT
is used correctly in thetradeContent1
object. Ensure that theTradeType
enum is used consistently throughout the codebase.423-427: The
TradeType.LIMIT
is used correctly in thetradeContent2
object. Ensure that theTradeType
enum is used consistently throughout the codebase.437-441: The
TradeType.LIMIT
is used correctly in thetradeContent3
object. Ensure that theTradeType
enum is used consistently throughout the codebase.indexer/services/comlink/src/types.ts (2)
16-21: The
TradeType
has been added to the import statement. Ensure that theTradeType
enum is correctly defined in the@dydxprotocol-indexer/postgres
module.174-180: The
type
property inTradeResponseObject
has been changed fromFillType
toTradeType
. Make sure that all instances whereTradeResponseObject
is used have been updated to handleTradeType
instead ofFillType
.indexer/services/comlink/src/request-helpers/request-transformer.ts (2)
4-10: The new function
fillTypeToTradeType
has been added to the import statement. Ensure that this function is correctly implemented and tested.177-183: The
type
property of thefill
object is now being processed through thefillTypeToTradeType
function. This suggests a change in how thetype
property is handled in the trade response object. Make sure that thefillTypeToTradeType
function correctly convertsFillType
toTradeType
.indexer/services/ender/__tests__/helpers/constants.ts (2)
1-5: The import statement for
TradeType
has been updated correctly. Ensure that theTradeType
enum is correctly defined in the@dydxprotocol-indexer/postgres
module.325-329: The
type
property ofdefaultTradeContent
has been updated to useTradeType.LIMIT
. Ensure that all instances ofdefaultTradeContent
in the codebase are updated to handle this change.indexer/services/comlink/public/api-documentation.md (2)
3052-3077: The new
TradeType
enum is well defined with the valuesLIMIT
,LIQUIDATED
, andDELEVERAGED
. Ensure that these values are correctly handled in the codebase where this enum is used.3106-3106: The
type
property inTradeResponseObject
now referencesTradeType
instead ofFillType
. Make sure that all instances ofTradeResponseObject
in the codebase are updated to handleTradeType
.indexer/services/comlink/public/websocket-documentation.md (3)
269-276: The new
TradeType
enum is well defined and the comments provide clear explanations for each type. Ensure that these new types are correctly handled in the rest of the codebase.724-724: The
type
property in theTradeContent
interface has been updated to use theTradeType
enum. Ensure that all instances ofTradeContent
in the codebase have been updated accordingly.746-754: The example JSON response has been updated to use the new
TradeType
enum. Ensure that the actual response from the server matches this example.
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: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- indexer/services/comlink/src/request-helpers/request-transformer.ts (2 hunks)
- indexer/services/comlink/src/types.ts (2 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/comlink/src/types.ts
Additional comments: 2
indexer/services/comlink/src/request-helpers/request-transformer.ts (2)
5-11: The new function
fillTypeToTradeType
is imported but there is no check for its return value. Ensure that it always returns a validTradeType
for all possibleFillType
values.178-184: The
fillToTradeResponseObject
function now usesfillTypeToTradeType(fill.type)
to determine thetype
property. Ensure that this change does not break any code that relies on the previoustype
value.- type: fill.type, + type: fillTypeToTradeType(fill.type),
Changelist
add new trade type
Test Plan
unit tests
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.