Sp/vault post only orders #2198
Conversation
WalkthroughThe changes introduce modifications to the Changes
Poem
Tip Announcements
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
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 using PR comments)
Other keywords and placeholders
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: 1
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/vault/keeper/orders_test.go (1)
Line range hint
462-801: Approve the enhancements to the test function, suggest adding more test cases.The modifications to the
TestGetVaultClobOrdersfunction are well-implemented, enhancing the test coverage for the new "post-only" order logic. The use of theexpectedTimeInForcefield to specify expected conditions is a robust method for ensuring that the functionality behaves as intended.Consider adding additional test cases to cover edge scenarios that may not be fully tested by the current set, such as orders that are close to the threshold conditions for setting "post-only" status.
If needed, I can help in defining additional test cases or refining the existing ones to ensure comprehensive coverage.
| // If the side would increase the vault's inventory, make the order post-only. | ||
| timeInForceType := clobtypes.Order_TIME_IN_FORCE_UNSPECIFIED | ||
| if (side == clobtypes.Order_SIDE_SELL && inventory.Sign() <= 0) || | ||
| (side == clobtypes.Order_SIDE_BUY && inventory.Sign() >= 0) { | ||
| timeInForceType = clobtypes.Order_TIME_IN_FORCE_POST_ONLY | ||
| } | ||
|
|
||
| return &clobtypes.Order{ | ||
| OrderId: *orderId, | ||
| Side: side, | ||
| Quantums: orderSize.Uint64(), // Validated to be a uint64 above. | ||
| Subticks: subticksRounded, | ||
| GoodTilOneof: goodTilBlockTime, | ||
| TimeInForce: timeInForceType, |
There was a problem hiding this comment.
Approve the logic addition, suggest enhancing documentation.
The changes made to the GetVaultClobOrders function correctly implement the new "post-only" order logic based on the vault's inventory status and order side. This is a crucial update for preventing unintended inventory increases.
However, it would be beneficial to add more comprehensive comments explaining this logic within the code to enhance future maintainability and clarity for other developers or reviewers who might not be familiar with the context of these changes.
Consider adding detailed comments like the following:
+ // Set the order to "post-only" if it would increase the vault's inventory.
+ // This is determined by checking if the order side is "sell" and the inventory is non-positive,
+ // or if the order side is "buy" and the inventory is non-negative.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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If the side would increase the vault's inventory, make the order post-only. | |
| timeInForceType := clobtypes.Order_TIME_IN_FORCE_UNSPECIFIED | |
| if (side == clobtypes.Order_SIDE_SELL && inventory.Sign() <= 0) || | |
| (side == clobtypes.Order_SIDE_BUY && inventory.Sign() >= 0) { | |
| timeInForceType = clobtypes.Order_TIME_IN_FORCE_POST_ONLY | |
| } | |
| return &clobtypes.Order{ | |
| OrderId: *orderId, | |
| Side: side, | |
| Quantums: orderSize.Uint64(), // Validated to be a uint64 above. | |
| Subticks: subticksRounded, | |
| GoodTilOneof: goodTilBlockTime, | |
| TimeInForce: timeInForceType, | |
| // Set the order to "post-only" if it would increase the vault's inventory. | |
| // This is determined by checking if the order side is "sell" and the inventory is non-positive, | |
| // or if the order side is "buy" and the inventory is non-negative. | |
| timeInForceType := clobtypes.Order_TIME_IN_FORCE_UNSPECIFIED | |
| if (side == clobtypes.Order_SIDE_SELL && inventory.Sign() <= 0) || | |
| (side == clobtypes.Order_SIDE_BUY && inventory.Sign() >= 0) { | |
| timeInForceType = clobtypes.Order_TIME_IN_FORCE_POST_ONLY | |
| } | |
| return &clobtypes.Order{ | |
| OrderId: *orderId, | |
| Side: side, | |
| Quantums: orderSize.Uint64(), // Validated to be a uint64 above. | |
| Subticks: subticksRounded, | |
| GoodTilOneof: goodTilBlockTime, | |
| TimeInForce: timeInForceType, |
Changelist
What is the change:
Reason for change:
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
TimeInForcebased on inventory status, improving order processing in the vault system.Bug Fixes
TimeInForceconditions in various scenarios.