[CLOB-1007] add protos for final settlement clob pair status and removal reason #828
Conversation
jakob-dydx
commented
Dec 1, 2023
jakob-dydx
commented
- adds new FINAL_SETTLEMENT clob pair status
- adds equivalent indexer clob pair status
- adds indexer order removal reason for final settlement (will be used when forcefully-cancelling open stateful orders)
WalkthroughThe updates across various files and protocols introduce a new status called 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 ignored due to filter (3)
- protocol/indexer/protocol/v1/clob.pb.go
- protocol/indexer/shared/removal_reason.pb.go
- protocol/x/clob/types/clob_pair.pb.go
Files selected for processing (7)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/clob_pair.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/clob.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/shared/removal_reason.ts (4 hunks)
- proto/dydxprotocol/clob/clob_pair.proto (1 hunks)
- proto/dydxprotocol/indexer/protocol/v1/clob.proto (1 hunks)
- proto/dydxprotocol/indexer/shared/removal_reason.proto (1 hunks)
- protocol/indexer/protocol/v1/v1_mappers.go (1 hunks)
Additional comments: 14
indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/clob_pair.ts (4)
37-41: The addition of STATUS_FINAL_SETTLEMENT to the ClobPair_Status enum is correctly implemented and follows the existing pattern of enum values.
78-82: The addition of STATUS_FINAL_SETTLEMENT to the ClobPair_StatusSDKType enum is correctly implemented and follows the existing pattern of enum values.
111-113: The clobPair_StatusFromJSON function has been correctly updated to include the new STATUS_FINAL_SETTLEMENT status.
141-142: The clobPair_StatusToJSON function has been correctly updated to include the new STATUS_FINAL_SETTLEMENT status.
indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/clob.ts (4)
311-317: The addition of the new enum value
CLOB_PAIR_STATUS_FINAL_SETTLEMENTis correctly implemented and documented.360-366: The addition of the new enum value
CLOB_PAIR_STATUS_FINAL_SETTLEMENTto theClobPairStatusSDKTypeenum is correctly implemented and documented.392-397: The
clobPairStatusFromJSONfunction has been updated to handle the newCLOB_PAIR_STATUS_FINAL_SETTLEMENTenum value correctly.425-426: The
clobPairStatusToJSONfunction has been updated to handle the newCLOB_PAIR_STATUS_FINAL_SETTLEMENTenum value correctly.indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/shared/removal_reason.ts (1)
- 79-79: The comment update for
ORDER_REMOVAL_REASON_FINAL_SETTLEMENTis clear and accurately reflects the reason for an order's removal due to the final settlement of its ClobPair.proto/dydxprotocol/clob/clob_pair.proto (1)
- 68-70: The addition of
STATUS_FINAL_SETTLEMENTto theStatusenum is consistent with the summary provided. Ensure that all associated systems and documentation are updated to reflect this new status.proto/dydxprotocol/indexer/protocol/v1/clob.proto (1)
- 176-180: The addition of the
CLOB_PAIR_STATUS_FINAL_SETTLEMENTenum value is correctly implemented and follows the protocol's pattern for defining statuses.proto/dydxprotocol/indexer/shared/removal_reason.proto (2)
53-55: The addition of
ORDER_REMOVAL_REASON_FINAL_SETTLEMENTto theOrderRemovalReasonenum is consistent with the summary and correctly follows the enum declaration syntax in Protocol Buffers.51-56: No further issues or improvements are identified in the provided hunk. The new enum constant is added with a unique value, and the comments are clear and descriptive.
protocol/indexer/protocol/v1/v1_mappers.go (1)
- 176-182: The addition of the new case
clobtypes.ClobPair_STATUS_FINAL_SETTLEMENTin theConvertToClobPairStatusfunction correctly maps the new status to the correspondingClobPairStatusenum value. Ensure that all other parts of the system that interact with ClobPair statuses are updated to handle this new status appropriately.
| @@ -65,6 +65,9 @@ message ClobPair { | |||
| // Clob pairs in this state only accept orders which are | |||
| // both short-term and post-only. | |||
| STATUS_INITIALIZING = 5; | |||
| // STATUS_FINAL_SETTLEMENT represents a clob pair which is deactivated | |||
| // and trading has ceased. All open positions will be closed by the protocol.option | |||
There was a problem hiding this comment.
There appears to be a typo in the comment on line 69. The word 'option' should be removed as it seems to be out of place.
- // and trading has ceased. All open positions will be closed by the protocol.option
+ // and trading has ceased. All open positions will be closed by the protocol.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.
| // and trading has ceased. All open positions will be closed by the protocol.option | |
| // and trading has ceased. All open positions will be closed by the protocol. |
| default: | ||
| panic("invalid clob pair status") |
There was a problem hiding this comment.
Using panic in the default case of the switch statement can be dangerous as it will cause the application to crash if an unexpected status is encountered. Consider handling this case more gracefully, possibly by returning an error that can be handled by the caller, to avoid crashing the entire application.
|
|
||
| /** | ||
| * STATUS_FINAL_SETTLEMENT - STATUS_FINAL_SETTLEMENT represents a clob pair which is deactivated | ||
| * and trading has ceased. All open positions will be closed by the protocol.option |
There was a problem hiding this comment.
There appears to be a typo in the comment. "protocol.option" should be corrected to "protocol."
- * and trading has ceased. All open positions will be closed by the protocol.option
+ * and trading has ceased. All open positions will be closed by the protocol.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.
| * and trading has ceased. All open positions will be closed by the protocol.option | |
| * and trading has ceased. All open positions will be closed by the protocol. |
| @@ -155,6 +158,9 @@ export enum OrderRemovalReasonSDKType { | |||
| * equity tier requirements. | |||
| */ | |||
| ORDER_REMOVAL_REASON_EQUITY_TIER = 13, | |||
|
|
|||
| /** ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed since its ClobPair has entered final settlement. */ | |||
There was a problem hiding this comment.
The comment for ORDER_REMOVAL_REASON_FINAL_SETTLEMENT in the OrderRemovalReasonSDKType enum should be updated to match the new wording in the OrderRemovalReason enum for consistency.
- /** ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed since its ClobPair has entered final settlement. */
+ /** ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed due to final settlement of its ClobPair. */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.
| /** ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed since its ClobPair has entered final settlement. */ | |
| /** ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed due to final settlement of its ClobPair. */ |
| @@ -215,6 +221,10 @@ | |||
| case "ORDER_REMOVAL_REASON_EQUITY_TIER": | |||
| return OrderRemovalReason.ORDER_REMOVAL_REASON_EQUITY_TIER; | |||
|
|
|||
| case 14: | |||
| case "ORDER_REMOVAL_REASON_FINAL_SETTLEMENT": | |||
There was a problem hiding this comment.
The comment for ORDER_REMOVAL_REASON_FINAL_SETTLEMENT in the orderRemovalReasonFromJSON and orderRemovalReasonToJSON functions should be updated to match the new wording in the OrderRemovalReason enum for consistency.
- case "ORDER_REMOVAL_REASON_FINAL_SETTLEMENT":
+ case "ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed due to final settlement of its ClobPair.":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.
| case "ORDER_REMOVAL_REASON_FINAL_SETTLEMENT": | |
| case "ORDER_REMOVAL_REASON_FINAL_SETTLEMENT - The order has been removed due to final settlement of its ClobPair.": |
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/constants.ts (1 hunks)
- indexer/packages/postgres/src/types/perpetual-market-types.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/packages/postgres/src/types/perpetual-market-types.ts
Additional comments: 1
indexer/packages/postgres/src/constants.ts (1)
- 110-113: The addition of the
CLOB_PAIR_STATUS_FINAL_SETTLEMENTmapping toPerpetualMarketStatus.FINAL_SETTLEMENTis consistent with the summary. Please verify that all parts of the code that useCLOB_STATUS_TO_MARKET_STATUShave been updated to handle the newFINAL_SETTLEMENTstatus appropriately.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (3)
- indexer/services/comlink/public/swagger.json
- protocol/indexer/protocol/v1/clob.pb.go
- protocol/x/clob/types/clob_pair.pb.go
Files selected for processing (5)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/clob_pair.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/clob.ts (4 hunks)
- indexer/services/comlink/public/api-documentation.md (1 hunks)
- proto/dydxprotocol/clob/clob_pair.proto (1 hunks)
- proto/dydxprotocol/indexer/protocol/v1/clob.proto (1 hunks)
Files skipped from review due to trivial changes (1)
- proto/dydxprotocol/indexer/protocol/v1/clob.proto
Files skipped from review as they are similar to previous changes (3)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/clob/clob_pair.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/protocol/v1/clob.ts
- proto/dydxprotocol/clob/clob_pair.proto
Additional comments: 1
indexer/services/comlink/public/api-documentation.md (1)
- 2819-2819: The addition of the
FINAL_SETTLEMENTstate to thePerpetualMarketStatusschema is consistent with the changes described in the summary and pull request details.
jakob-dydx
force-pushed
the
jakob-dydx/final-settlement
branch
from
25db9e4 to
3131060
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/scripts/dydx_clob_pair_status_to_market_status.sql (1 hunks)
Additional comments: 1
indexer/services/ender/src/scripts/dydx_clob_pair_status_to_market_status.sql (1)
- 17-23: The addition of the 'FINAL_SETTLEMENT' status with the case '6'::jsonb aligns with the PR objective and the provided summary. Ensure that all dependent systems are updated to handle this new status.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/indexer/protocol/v1/v1_mappers_test.go (1 hunks)
Additional comments: 1
protocol/indexer/protocol/v1/v1_mappers_test.go (1)
- 398-407: The changes from lines 398 to 407 update the expected panic message in the
TestConvertToClobPairStatustest case to include the specific invalid clob pair status value. This is a good practice as it provides more detailed information when the test fails, making it easier to debug issues.
