Full Node Status taker order status protos #2003
Conversation
WalkthroughThe recent updates enhance the dydxprotocol by introducing new message definitions that improve how liquidation and taker orders are handled. The Changes
Poem
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:
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 Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
protocol/x/clob/types/order.pb.gois excluded by!**/*.pb.goprotocol/x/clob/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (2)
- proto/dydxprotocol/clob/order.proto (2 hunks)
- proto/dydxprotocol/clob/query.proto (1 hunks)
Additional comments not posted (3)
proto/dydxprotocol/clob/query.proto (2)
240-255: LGTM! But verify the message usage in the codebase.The code changes are approved.
However, ensure that all usages of
StreamTakerOrderStatusare correctly integrated within the codebase.Verification successful
The
StreamTakerOrderStatusmessage is correctly integrated within theStreamTakerOrdermessage.
StreamTakerOrderStatusis used as a field within theStreamTakerOrdermessage, indicating proper integration.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `StreamTakerOrderStatus` in the codebase. # Test: Search for the message usage. Expect: Correct integration of `StreamTakerOrderStatus`. rg --type proto -A 5 $'StreamTakerOrderStatus'Length of output: 75
Script:
#!/bin/bash # Description: Verify all usages of `StreamTakerOrderStatus` in the codebase. # Locate all .proto files and search for the message usage within them. fd -e proto -x rg 'StreamTakerOrderStatus' {}Length of output: 215
Script:
#!/bin/bash # Description: Verify the context of the `StreamTakerOrderStatus` field declaration in the codebase. # Locate all .proto files and search for the context around the field declaration. fd -e proto -x rg -A 10 'StreamTakerOrderStatus taker_order_status = 3;' {}Length of output: 557
Script:
#!/bin/bash # Description: Verify the enclosing message or service definition for the `StreamTakerOrderStatus` field declaration in the codebase. # Locate all .proto files and search for the enclosing message or service definition around the field declaration. fd -e proto -x rg -B 10 -A 10 'StreamTakerOrderStatus taker_order_status = 3;' {}Length of output: 909
224-238: LGTM! But verify the message usage in the codebase.The code changes are approved.
However, ensure that all usages of
StreamTakerOrderare correctly integrated within the codebase.Verification successful
Usages of
StreamTakerOrderare correctly integrated.The
StreamTakerOrdermessage is extensively used in theprotocol/x/clob/types/query.pb.gofile, and it appears to be correctly integrated within the codebase.
protocol/x/clob/types/query.pb.go: Multiple references toStreamTakerOrder, including its definition, methods, and related structures.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `StreamTakerOrder` in the codebase. # Test: Search for the message usage. Expect: Correct integration of `StreamTakerOrder`. rg --type proto -A 5 $'StreamTakerOrder'Length of output: 69
Script:
#!/bin/bash # Description: Verify all usages of `StreamTakerOrder` in the codebase. # Test: Search for the message usage. Expect: Correct integration of `StreamTakerOrder`. rg -A 5 'StreamTakerOrder' # Test: Search for the message definition using ast-grep. Expect: Locate the definition of `StreamTakerOrder`. ast-grep --lang proto --pattern $'message StreamTakerOrder { $$$ }'Length of output: 21003
proto/dydxprotocol/clob/order.proto (1)
232-249: LGTM! But verify the message usage in the codebase.The code changes are approved.
However, ensure that all usages of
StreamLiquidationOrderare correctly integrated within the codebase.Verification successful
The
StreamLiquidationOrdermessage is correctly integrated within the codebase.The message
StreamLiquidationOrderis referenced in multiple files, indicating proper integration.
protocol/x/clob/types/query.pb.goprotocol/x/clob/types/order.pb.goproto/dydxprotocol/clob/query.protoproto/dydxprotocol/clob/order.protoScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all usages of `StreamLiquidationOrder` in the codebase. # Test: Search for the message usage. Expect: Correct integration of `StreamLiquidationOrder`. rg --type proto -A 5 $'StreamLiquidationOrder'Length of output: 75
Script:
#!/bin/bash # Description: Verify all usages of `StreamLiquidationOrder` in the codebase. # Locate all `.proto` files and search for the message usage within those files. fd --extension proto --exec rg 'StreamLiquidationOrder' {}Length of output: 225
Script:
#!/bin/bash # Description: Verify all usages of `StreamLiquidationOrder` in the codebase. # Search for the message usage across the entire codebase. rg 'StreamLiquidationOrder'Length of output: 3537
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (1)
- proto/dydxprotocol/clob/query.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/dydxprotocol/clob/query.proto
| // It is intended to be used only in full node streaming. | ||
| message StreamTakerOrderStatus { | ||
| // The state of the taker order after attempting to match it against the orderbook. | ||
| uint32 order_status = 1; |
There was a problem hiding this comment.
what are the possible values of order_status? Link to codebase?
There was a problem hiding this comment.
let's not link main since that is subject to change. link to some release or some commit
There was a problem hiding this comment.
also just to make sure - PO is the only thing missing from these enum values? status captures all possible results and we don't need the error, right?
proto/dydxprotocol/clob/query.proto
Outdated
| // The amount of remaining (non-matched) base quantums of this taker order. | ||
| uint64 remaining_quantums = 2; | ||
|
|
||
| // The amount of base quantums that were optimistically filled (from this current |
There was a problem hiding this comment.
indentation a little weird
There was a problem hiding this comment.
tabs vs 2 spaces lol
proto/dydxprotocol/clob/query.proto
Outdated
| uint64 remaining_quantums = 2; | ||
|
|
||
| // The amount of base quantums that were optimistically filled (from this current | ||
| // matching cycle) of this taker order. Note that if any quantums of this order |
There was a problem hiding this comment.
what's a current matching cycle? a proposed block?
There was a problem hiding this comment.
This wording is a little confusing to read imo. Perhaps:
// The amount of base quantums that were optimistically filled for this taker order during the proposed block. This doesn't include prior fills.
There was a problem hiding this comment.
Hmm that statement is incorrect since this is one invocation of the matching loop (in checkTx, optimistic matching). Processing a proposed block is in DeliverTx and we just record matches instead of placing orders. Updated the comment
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/clob/types/query.pb.gois excluded by!**/*.pb.go
Files selected for processing (3)
- proto/dydxprotocol/clob/query.proto (1 hunks)
- protocol/x/clob/types/liquidation_order.go (1 hunks)
- protocol/x/clob/types/orderbook.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/dydxprotocol/clob/query.proto
Additional comments not posted (2)
protocol/x/clob/types/liquidation_order.go (1)
54-64: LGTM! Ensure theStreamLiquidationOrderstruct is correctly defined.The method correctly maps and converts the fields from
LiquidationOrdertoStreamLiquidationOrder.However, verify that the
StreamLiquidationOrderstruct is correctly defined elsewhere in the codebase.Verification successful
The
StreamLiquidationOrderstruct is correctly defined.The
StreamLiquidationOrderstruct inprotocol/x/clob/types/order.pb.goincludes the necessary fields:LiquidationInfo,ClobPairId, andIsBuy, which are correctly mapped in theToStreamLiquidationOrdermethod.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `StreamLiquidationOrder` struct. # Test: Search for the struct definition. Expect: The struct is correctly defined. rg --type go -A 5 $'type StreamLiquidationOrder struct'Length of output: 775
protocol/x/clob/types/orderbook.go (1)
105-113: LGTM! Ensure theStreamTakerOrderStatusstruct is correctly defined.The method correctly maps and converts the fields from
TakerOrderStatustoStreamTakerOrderStatus.However, verify that the
StreamTakerOrderStatusstruct is correctly defined elsewhere in the codebase.Verification successful
Ensure all fields are correctly defined in the
StreamTakerOrderStatusstruct.The initial part of the
StreamTakerOrderStatusstruct definition looks correct. Verify that it includes all the fields mapped in theToStreamingTakerOrderStatusmethod:OrderStatus,RemainingQuantums, andOptimisticallyFilledQuantums.
Verify the presence of the
OptimisticallyFilledQuantumsfield in theStreamTakerOrderStatusstruct.The
StreamTakerOrderStatusstruct includesOrderStatusandRemainingQuantums. Ensure that it also includesOptimisticallyFilledQuantums.
The
StreamTakerOrderStatusstruct is correctly defined.The
StreamTakerOrderStatusstruct includes all the necessary fields:OrderStatus,RemainingQuantums, andOptimisticallyFilledQuantums.
OrderStatus(uint32)RemainingQuantums(uint64)OptimisticallyFilledQuantums(uint64)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition of `StreamTakerOrderStatus` struct. # Test: Search for the struct definition. Expect: The struct is correctly defined. rg --type go -A 5 $'type StreamTakerOrderStatus struct'Length of output: 716
Script:
#!/bin/bash # Description: Verify the complete definition of `StreamTakerOrderStatus` struct. # Test: Search for the full struct definition. Expect: The struct includes all required fields. rg --type go -A 10 'type StreamTakerOrderStatus struct' protocol/x/clob/types/query.pb.goLength of output: 1002
Script:
#!/bin/bash # Description: Verify the complete definition of `StreamTakerOrderStatus` struct. # Test: Search for the full struct definition. Expect: The struct includes all required fields. rg --type go -A 20 'type StreamTakerOrderStatus struct' protocol/x/clob/types/query.pb.goLength of output: 1595
| // The amount of remaining (non-matched) base quantums of this taker order. | ||
| uint64 remaining_quantums = 2; |
There was a problem hiding this comment.
for my understanding - this is the quantums remaining after optimisitic fills?
There was a problem hiding this comment.
yes, after the matching loop, remaining quantums
| uint32 clob_pair_id = 2; | ||
|
|
||
| // True if this is a buy order liquidating a short position, false if vice versa. | ||
| bool is_buy = 3; |
There was a problem hiding this comment.
should we use Side similar to Order for consistency?
no strong preference though
There was a problem hiding this comment.
Hmm I think it would be a little weird since we nest Side in the Order proto currently, and if we change it to Side for liquidation order we would have to nest it in the Liquidation order proto. I'll keep it as is for minimal changes.
Summary by CodeRabbit
StreamLiquidationOrdermessage to enhance liquidation order management in the trading protocol.StreamTakerOrderandStreamTakerOrderStatusmessages to improve order matching capabilities and provide real-time feedback on order statuses.