Orderbook Fills emission #1448
Orderbook Fills emission #1448
Conversation
WalkthroughThe recent updates focus on enhancing the orderbook streaming functionalities within a protocol. Key modifications include adding detailed comments, introducing new interfaces for orderbook updates and fills, and implementing methods across various components to handle these updates efficiently. The changes aim to streamline the process of sending and managing orderbook fills through gRPC streams, improving the robustness and clarity of the system's data handling capabilities. Changes
This table groups files with similar changes to provide a concise overview of the modifications made across the system. 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 Configration File (
|
| func (f *FakeMemClobKeeper) SendOrderbookFillUpdates( | ||
| ctx sdk.Context, | ||
| orderbookFills []types.StreamOrderbookFill, | ||
| ) { | ||
| } |
There was a problem hiding this comment.
The method SendOrderbookFillUpdates is currently empty.
This method is defined but does not contain any implementation. If this is intentional (e.g., for a mock or placeholder), it should be documented clearly. Otherwise, it should be implemented or removed if not needed.
protocol/x/clob/memclob/memclob.go
Outdated
| internalOperation := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders) | ||
| // If orderbook updates are on, send an orderbook update with the fill to grpc streams. | ||
| if m.generateOrderbookUpdates { | ||
| clobMatch := internalOperation.GetMatch() | ||
| orderbookMatchFill := m.GenerateStreamOrderbookFill(ctx, *clobMatch, takerOrder, makerFillWithOrders) | ||
| m.clobKeeper.SendOrderbookFillUpdates(ctx, []types.StreamOrderbookFill{orderbookMatchFill}) | ||
| } |
There was a problem hiding this comment.
The changes from lines 389 to 395 introduce a new logic to send orderbook updates with fills to GRPC streams if orderbook updates are enabled. This is a significant change as it directly affects how orderbook fills are communicated to external systems.
- internalOperation := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders)
+ internalOperation, err := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders)
+ if err != nil {
+ return nil, err
+ }Ensure that errors from MustAddMatchToOperationsQueue are handled appropriately to prevent potential issues during order matching.
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.
| internalOperation := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders) | |
| // If orderbook updates are on, send an orderbook update with the fill to grpc streams. | |
| if m.generateOrderbookUpdates { | |
| clobMatch := internalOperation.GetMatch() | |
| orderbookMatchFill := m.GenerateStreamOrderbookFill(ctx, *clobMatch, takerOrder, makerFillWithOrders) | |
| m.clobKeeper.SendOrderbookFillUpdates(ctx, []types.StreamOrderbookFill{orderbookMatchFill}) | |
| } | |
| internalOperation, err := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // If orderbook updates are on, send an orderbook update with the fill to grpc streams. | |
| if m.generateOrderbookUpdates { | |
| clobMatch := internalOperation.GetMatch() | |
| orderbookMatchFill := m.GenerateStreamOrderbookFill(ctx, *clobMatch, takerOrder, makerFillWithOrders) | |
| m.clobKeeper.SendOrderbookFillUpdates(ctx, []types.StreamOrderbookFill{orderbookMatchFill}) | |
| } |
b6f3203 to
ea262ba
Compare
ea262ba to
9c34203
Compare
| @@ -174,45 +174,6 @@ func PrepareCheckState( | |||
| offchainUpdates, | |||
| ) | |||
|
|
|||
| // For orders that are filled in the last block, send an orderbook update to the grpc streams. | |||
There was a problem hiding this comment.
Axed this whole section to send out order updates.
Reason being:
- PCS check state is set to deliver state, so no additional updates are needed.
- since fills are directly emitted immediately on orderbook update, matched orders don't need to be updated
| @@ -276,6 +276,19 @@ func (k Keeper) RemoveOrderFillAmount(ctx sdk.Context, orderId types.OrderId) { | |||
| []byte(types.OrderAmountFilledKeyPrefix), | |||
| ) | |||
| memStore.Delete(orderId.ToStateKey()) | |||
|
|
|||
| // If grpc stream is on, zero out the fill amount. | |||
There was a problem hiding this comment.
Zero'ing out fill amounts when fill amounts are removed. I added this to the innermost statewrite/remove method. I could have added it to each of the 3-4 callsites but this might be cleaner. Let me know if the other approach is preferred.
| } | ||
| } | ||
| k.SendOrderbookUpdates(ctx, allUpdates, false) | ||
| } |
There was a problem hiding this comment.
No need since this zero'ing out fills is punted to the innermost state remove method
| @@ -141,6 +141,63 @@ func (sm *GrpcStreamingManagerImpl) SendOrderbookUpdates( | |||
| } | |||
| } | |||
|
|
|||
| // SendOrderbookFillUpdates groups fills by their clob pair ids and | |||
| // sends messages to the subscribers. | |||
| func (sm *GrpcStreamingManagerImpl) SendOrderbookFillUpdates( | |||
There was a problem hiding this comment.
similar looking functions - worth to create helper?
nbd - will let you make the call
| orderId := shortTermOrderPlacement.GetOrder().OrderId | ||
| orderIdSet[orderId] = struct{}{} | ||
| } | ||
| if clobMatch := operation.GetMatch(); clobMatch != nil { |
There was a problem hiding this comment.
I added all the order ids involved in a match, but technically there were no errors in dev without this statement.
I think technically we can remove this since I think it's an invariant that every short term order Id that shows up in the any opqueue matches must appear before as a short term order placement. Additionally, there would be no short term order placements that show up without being involved in a match.
What do you think?
| // If grpc streams are on, send absolute fill amounts from local + proposed opqueue to the grpc stream. | ||
| // This must be sent out to account for checkState being discarded and deliverState being used. | ||
| if streamingManager := k.GetGrpcStreamingManager(); streamingManager.Enabled() { | ||
| localValidatorOperationsQueue, _ := k.MemClob.GetOperationsToReplay(ctx) |
There was a problem hiding this comment.
Venn diagram. Update both proposed and local. I believe we need both.
| streamingManager.SendOrderbookFillUpdates( | ||
| ctx, | ||
| []types.StreamOrderbookFill{ | ||
| streamOrderbookFill, |
There was a problem hiding this comment.
DeliverTx send fills out.
| m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders) | ||
| internalOperation := m.operationsToPropose.MustAddMatchToOperationsQueue(takerOrder, makerFillWithOrders) | ||
| // If orderbook updates are on, send an orderbook update with the fill to grpc streams. | ||
| if m.generateOrderbookUpdates { |
There was a problem hiding this comment.
Optimistic fill emission for any matches that occur on this validator
| updates := m.GetOrderbookUpdatesForOrderUpdate(ctx, fill.MakerOrderId) | ||
| allUpdates.Append(updates) | ||
| } | ||
| m.clobKeeper.SendOrderbookUpdates(ctx, allUpdates, false) |
There was a problem hiding this comment.
Removing this didn't trigger any errors, but i think we still should have this -- It makes sense that we need to reset fill amounts if state was discarded.
2 qns
- do we need to reset anything else other than the order ids in the fills?
- do we need to reset orderbook placements for your side of streaming?
There was a problem hiding this comment.
- I don't think so? think we only persist fills, right? other info (rewards, fees, etc) are irrelevent
- no - because orderbook updates are persisted and not reverted even with failed placement
| // If grpc streams are on, send absolute fill amounts from local + proposed opqueue to the grpc stream. | ||
| // This must be sent out to account for checkState being discarded and deliverState being used. | ||
| if streamingManager := k.GetGrpcStreamingManager(); streamingManager.Enabled() { | ||
| localValidatorOperationsQueue, _ := k.MemClob.GetOperationsToReplay(ctx) |
There was a problem hiding this comment.
so one issue that I just thought of is we are probably going to enable optimistic block execution in the near term, which means that the whole FinalizedBlock will be done in a separate go routine. Accessing k.MemClob here (and in other deliver state ABCI callbacks) is dangerous and can potentially lead to race conditions.
are there any alternatives here?
There was a problem hiding this comment.
we can merge to unblock first, but should prob keep this in mind. grpc streams and OE might not co-exist
* new protos * fix prev use case * proto lint * emit stream fills when clob match appended to opqueue * more explicit function signature * uint32 -> 64 * deliverTx process opqueue clob match emissions * bugfixes from feature branch to emit fill amounts correctly * pr comments * fix tests * fixup sample pregenesis json
|
@Mergifyio backport release/protocol/v4.1.x |
✅ Backports have been created
|
* new protos * fix prev use case * proto lint * emit stream fills when clob match appended to opqueue * more explicit function signature * uint32 -> 64 * deliverTx process opqueue clob match emissions * bugfixes from feature branch to emit fill amounts correctly * pr comments * fix tests * fixup sample pregenesis json (cherry picked from commit cffdb5f) # Conflicts: # protocol/mocks/PricesKeeper.go # protocol/scripts/genesis/sample_pregenesis.json # protocol/x/clob/memclob/memclob_purge_invalid_memclob_state_test.go
* new protos * fix prev use case * proto lint * emit stream fills when clob match appended to opqueue * more explicit function signature * uint32 -> 64 * deliverTx process opqueue clob match emissions * bugfixes from feature branch to emit fill amounts correctly * pr comments * fix tests * fixup sample pregenesis json
* new protos * fix prev use case * proto lint * emit stream fills when clob match appended to opqueue * more explicit function signature * uint32 -> 64 * deliverTx process opqueue clob match emissions * bugfixes from feature branch to emit fill amounts correctly * pr comments * fix tests * fixup sample pregenesis json
Emit orderbook fills through grpc stream.
Tested via
https://github.com/dydxprotocol/v4-chain/pull/1471/commits