[TRA-209] Skip various order placement logic for vault orders #1332
Conversation
WalkthroughThe updates involve enhancing the order placement functionality in a trading protocol to differentiate between internal and external orders. By introducing the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (11)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/x/clob/ante/clob.go (1 hunks)
- protocol/x/clob/ante/clob_test.go (4 hunks)
- protocol/x/clob/keeper/msg_server_place_order.go (2 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
- protocol/x/clob/keeper/orders.go (3 hunks)
- protocol/x/clob/keeper/orders_test.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (2 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Files not reviewed due to errors (1)
- (no review received)
Additional Context Used
Path-based Instructions (11)
protocol/x/vault/types/expected_keepers.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/types/clob_keeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/msg_server_place_order.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/ante/clob.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/ante/clob_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/msg_server_place_order_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/mocks/ClobKeeper.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/orders.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/clob/keeper/orders_test.go (1)
Pattern
**/**: Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Learnings (1)
protocol/x/clob/ante/clob_test.go (1)
User: jonfung-dydx" PR: dydxprotocol/v4-chain#1009 File: protocol/x/clob/memclob/memclob_place_order_test.go:4108-4108 Timestamp: 2024-02-05T20:01:55.204Z Learning: The `assertPlaceOrderOffchainMessages` function in `memclob_test_util.go` indeed requires `ctx` as a parameter for its operations, contrary to the initial comment made.
Additional comments not posted (14)
protocol/x/vault/types/expected_keepers.go (1)
29-29: The addition of theisInternalOrderparameter to theHandleMsgPlaceOrderfunction is aligned with the PR objectives and introduces a clear way to differentiate internal orders.protocol/x/clob/types/clob_keeper.go (2)
50-50: The addition of theisInternalOrderparameter to theHandleMsgPlaceOrderfunction is aligned with the PR objectives and introduces a clear way to differentiate internal orders.
60-64: The addition of theisInternalOrderparameter to thePlaceStatefulOrdermethod is aligned with the PR objectives and introduces a clear way to differentiate internal orders.protocol/x/clob/keeper/msg_server_place_order.go (1)
28-28: Ensure that the addition of theisInternalOrderparameter and the conditional logic based on it in theHandleMsgPlaceOrderfunction do not compromise the system's integrity by skipping necessary validations or metrics for internal orders.protocol/x/clob/ante/clob.go (1)
88-88: The addition of thefalseparameter to thePlaceStatefulOrderfunction call is aligned with the PR objectives and introduces a clear way to differentiate internal orders.protocol/x/vault/keeper/orders.go (1)
86-86: The introduction of thePlaceVaultClobOrdermethod and its usage in theRefreshVaultClobOrdersmethod is aligned with the PR objectives and introduces a clear way to differentiate internal orders.protocol/x/clob/ante/clob_test.go (4)
103-103: The addition of thefalseparameter to thePlaceStatefulOrdermethod calls in the test function is aligned with the PR objectives and introduces a clear way to differentiate internal orders.
117-117: The addition of thefalseparameter to thePlaceStatefulOrdermethod calls in the test function is aligned with the PR objectives and introduces a clear way to differentiate internal orders.
160-160: The addition of thefalseparameter to thePlaceStatefulOrdermethod calls in the test function is aligned with the PR objectives and introduces a clear way to differentiate internal orders.
189-189: The addition of thefalseparameter to thePlaceStatefulOrdermethod calls in the test function is aligned with the PR objectives and introduces a clear way to differentiate internal orders.protocol/x/vault/keeper/orders_test.go (1)
112-112: Replace direct calls toHandleMsgPlaceOrderwithPlaceVaultClobOrderto streamline the order placement process for vault orders.- err := tApp.App.ClobKeeper.HandleMsgPlaceOrder(ctx, order) + err := tApp.App.VaultKeeper.PlaceVaultClobOrder(ctx, order)This change correctly aligns with the PR objectives to optimize the order placement process for vault orders by introducing a more streamlined method for placing these orders.
protocol/mocks/ClobKeeper.go (1)
962-972: Ensure that the newisInternalOrderparameter inPlaceStatefulOrderis properly documented to explain its purpose and effect on the function's behavior.protocol/x/clob/keeper/orders.go (2)
358-394: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [307-391]
Consider adding a comment explaining the purpose of the
isInternalOrderparameter for future maintainability.func (k Keeper) PlaceStatefulOrder( ctx sdk.Context, msg *types.MsgPlaceOrder, + // isInternalOrder indicates if the order is internal to the protocol, which affects certain checks. isInternalOrder bool, ) (err error) {
358-394: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [307-391]
Ensure that all external calls to
PlaceStatefulOrderhave been updated to include the newisInternalOrderparameter.Verification successful
The verification process did not find any calls to
PlaceStatefulOrderwith exactly three parameters, suggesting that all external calls to this function have been correctly updated to include the newisInternalOrderparameter. This supports the conclusion that the review comment has been addressed in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to PlaceStatefulOrder without the isInternalOrder parameter. Expecting no results. ast-grep --lang go --pattern $'PlaceStatefulOrder($_, $_, $_)'Length of output: 63
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (11)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/x/clob/ante/clob.go (1 hunks)
- protocol/x/clob/ante/clob_test.go (4 hunks)
- protocol/x/clob/keeper/msg_server_place_order.go (2 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
- protocol/x/clob/keeper/orders.go (3 hunks)
- protocol/x/clob/keeper/orders_test.go (1 hunks)
- protocol/x/clob/types/clob_keeper.go (2 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- protocol/mocks/ClobKeeper.go
- protocol/x/clob/ante/clob.go
- protocol/x/clob/ante/clob_test.go
- protocol/x/clob/keeper/msg_server_place_order.go
- protocol/x/clob/keeper/msg_server_place_order_test.go
- protocol/x/clob/keeper/orders.go
- protocol/x/clob/keeper/orders_test.go
- protocol/x/clob/types/clob_keeper.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/types/expected_keepers.go
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (10)
- protocol/mocks/ClobKeeper.go (3 hunks)
- protocol/x/clob/ante/clob.go (1 hunks)
- protocol/x/clob/ante/clob_test.go (4 hunks)
- protocol/x/clob/keeper/msg_server_place_order.go (2 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
- protocol/x/clob/keeper/orders.go (3 hunks)
- protocol/x/clob/types/clob_keeper.go (2 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
- protocol/x/vault/keeper/orders_test.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- protocol/mocks/ClobKeeper.go
- protocol/x/clob/ante/clob.go
- protocol/x/clob/ante/clob_test.go
- protocol/x/clob/keeper/msg_server_place_order.go
- protocol/x/clob/keeper/msg_server_place_order_test.go
- protocol/x/clob/keeper/orders.go
- protocol/x/clob/types/clob_keeper.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/types/expected_keepers.go
protocol/x/vault/keeper/orders.go
Outdated
| err := k.clobKeeper.HandleMsgPlaceOrder(ctx, clobtypes.NewMsgPlaceOrder(*order), true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
| if !isInternalOrder { | ||
| cancelledOrderIds := lib.UniqueSliceToSet(processProposerMatchesEvents.PlacedStatefulCancellationOrderIds) | ||
| if _, found := cancelledOrderIds[order.GetOrderId()]; found { | ||
| return errorsmod.Wrapf( | ||
| types.ErrStatefulOrderPreviouslyCancelled, | ||
| "PlaceOrder: order (%+v)", | ||
| order, | ||
| ) | ||
| } | ||
| removedOrderIds := lib.UniqueSliceToSet(processProposerMatchesEvents.RemovedStatefulOrderIds) | ||
| if _, found := removedOrderIds[order.GetOrderId()]; found { | ||
| return errorsmod.Wrapf( | ||
| types.ErrStatefulOrderPreviouslyRemoved, | ||
| "PlaceOrder: order (%+v)", | ||
| order, | ||
| ) | ||
| } |
There was a problem hiding this comment.
on a second thought - this isn't too expensive, right? let's maybe keep this to keep the logic consistent?
| // Attach various logging tags relative to this request. These should be static with no changes. | ||
| ctx = log.AddPersistentTagsToLogger(ctx, | ||
| log.Module, log.Clob, | ||
| log.ProposerConsAddress, sdk.ConsAddress(ctx.BlockHeader().ProposerAddress), | ||
| log.Callback, lib.TxMode(ctx), | ||
| log.BlockHeight, ctx.BlockHeight(), | ||
| log.Handler, log.PlaceOrder, | ||
| log.Msg, msg, |
There was a problem hiding this comment.
let's also keep the persistent tags at the for the initial version. in case we have issues this helps us debug
can skip the defered func below
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/keeper/msg_server_place_order.go (4 hunks)
- protocol/x/clob/keeper/msg_server_place_order_test.go (2 hunks)
- protocol/x/vault/keeper/orders.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/clob/keeper/msg_server_place_order.go
- protocol/x/clob/keeper/msg_server_place_order_test.go
- protocol/x/vault/keeper/orders.go
* Skip various order placement logic for vault orders
|
@Mergifyio backport release/protocol/v5.x |
✅ Backports have been created
|
* Skip various order placement logic for vault orders (cherry picked from commit 8e3ac79)
Changelist
skip following for vault orders: persistent tags, deferred logs / metrics, cancellation / removal check, equity tier limit check, collateralization check
Test Plan
existing / new unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.