add quoting logic of deactivated, stand-by, close-only modes of vaults #2299
Conversation
WalkthroughThe changes primarily focus on enhancing the management of vault orders within the protocol. Key modifications include improved conditions for order placement based on vault status, refined order cancellation logic, and updates to the retrieval of vault and quoting parameters. New functions have been introduced for better organization, while existing tests have been updated to cover additional scenarios and ensure robustness. Overall, these changes aim to improve the accuracy and efficiency of vault order handling. Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (12)
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: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- protocol/x/vault/keeper/orders.go (8 hunks)
- protocol/x/vault/keeper/orders_test.go (30 hunks)
- protocol/x/vault/keeper/params.go (1 hunks)
- protocol/x/vault/keeper/params_test.go (2 hunks)
- protocol/x/vault/keeper/withdraw.go (1 hunks)
Additional comments not posted (9)
protocol/x/vault/keeper/params.go (1)
97-119: LGTM! The changes improve the clarity and utility of the function.The modifications to
GetVaultAndQuotingParamsenhance its usability and provide more comprehensive information to callers:
- Renaming the function to
GetVaultAndQuotingParamsbetter reflects its expanded functionality of returning both vault parameters and quoting parameters.- Updating the return values to include both
vaultParamsandquotingParamsensures that callers receive all relevant information.- Returning the default quoting parameters when
vaultParams.QuotingParamsisnilguarantees that callers always receive quoting parameters, even if not specifically set for the vault.Overall, these changes make the function more robust and informative, improving its utility within the codebase.
protocol/x/vault/keeper/params_test.go (1)
Line range hint
149-199: LGTM!The test function
TestGetVaultAndQuotingParamsis well-structured and covers the necessary scenarios for retrieving vault and quoting parameters. The test cases verify the behavior with default quoting params, custom quoting params, and non-existent vault params. The assertions are correct and ensure the expected values are returned.protocol/x/vault/keeper/withdraw.go (1)
58-58: LGTM! The change adapts to the refactored function signature while preserving the original functionality.The updated function call to
GetVaultAndQuotingParamssuggests a refactoring in how vault-related data is accessed. The change maintains the retrieval of quoting parameters and the existence check, discarding the additional vault value that is not used in this context. The logic flow remains intact, ensuring the correct behavior of theGetVaultWithdrawalSlippagefunction.protocol/x/vault/keeper/orders.go (6)
37-38: HandlesCLOSE_ONLYstatus in order refresh logicThe updated condition now includes
VAULT_STATUS_CLOSE_ONLY, ensuring that vaults in bothQUOTINGandCLOSE_ONLYstatuses are processed correctly during the order refresh.
110-111: Improved order replacement condition by includingSubticksBy adding the comparison of
Subticks, the logic ensures that orders are replaced when there's a price change, enhancing accuracy in order management.
130-145: Cancellation of unnecessary orders to maintain order accuracyThe added logic correctly cancels orders that are no longer needed, preventing stale orders from remaining in the order book and ensuring the vault's orders reflect the current state.
206-212: Prevents order placement when vault is inactive or has no positionThe function now returns no orders if the vault is
DEACTIVATED,STAND_BY, orCLOSE_ONLYwith zero leverage, which aligns with expected behavior to avoid unnecessary order placement.
370-389: Generates reduce-only orders inCLOSE_ONLYmodeWhen the vault is in
CLOSE_ONLYstatus with a non-zero leverage, the code correctly generates reduce-only orders to close existing positions. It adjusts the order quantities based on the vault's inventory, ensuring orders do not exceed the position size.
Line range hint
414-418: Ensures quoting parameters exist before generating order IDsBy checking the existence of quoting parameters, the function avoids potential errors due to missing parameters, ensuring that order IDs are generated only when all necessary data is available.
protocol/x/vault/keeper/orders.go
Outdated
| i = 1 | ||
| } | ||
| for ; i < len(orders); i += 2 { | ||
| reduceOnlyOrders[i/2] = orders[i] |
There was a problem hiding this comment.
nit: Rather than do the division, maybe just keep another index counter that you increment per loop iteration?
There was a problem hiding this comment.
changed to using closeOnlySide to filter. no more indexing
protocol/x/vault/keeper/orders.go
Outdated
| stepSize := lib.BigU(clobPair.StepBaseQuantums) | ||
| reduceOnlyMaxOrderSize.Quo(reduceOnlyMaxOrderSize, stepSize) | ||
| reduceOnlyMaxOrderSize.Mul(reduceOnlyMaxOrderSize, stepSize) | ||
| // If vault is long, only need sell orders (even indices). |
There was a problem hiding this comment.
non-blocking: I wonder if this code could be built slightly more future-proof by setting some closeOnlySide based on long/short, then filtering out the orders from the generated orders rather than using even/odd indices.
| }, | ||
| }, | ||
| }, | ||
| "Success - Orders refresh due to status changing to close-only. No more orders": { |
There was a problem hiding this comment.
nit: Note that the vault has no position in this test-case?
| }, | ||
| }, | ||
| }, | ||
| "Success - Orders refresh due to status changing to close-only. Sell orders only": { |
There was a problem hiding this comment.
nit: Another similar test-case for STAND_BY and DEACTIVATED?
There was a problem hiding this comment.
right now we simply skip refresh orders if status is stand_by or deactivated (no order is cancelled) . I'm planning to have another change that cancels existing orders if status is changed to stand_by or deactivated and will add these test cases in that change
| 41_666_000, | ||
| }, | ||
| }, | ||
| "Success - Vault Clob 1, close-only status, 2 layers, leverage 0.6, sell orders only, order size": { |
There was a problem hiding this comment.
nit: Update title to indicate this is for capped order size = position size.
| reduceOnlyOrders := make([]*clobtypes.Order, quotingParams.Layers) | ||
| reduceOnlyMaxOrderSize := k.GetVaultInventoryInPerpetual(ctx, vaultId, perpetual.Params.Id) | ||
| stepSize := lib.BigU(clobPair.StepBaseQuantums) | ||
| reduceOnlyMaxOrderSize.Quo(reduceOnlyMaxOrderSize, stepSize) |
There was a problem hiding this comment.
nit: Return no orders if this is 0?
Changelist
in deactivated and stand-by mode, don't quote
in close-only mode: quote only if short or long
in another PR, i'm thinking:
Test Plan
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.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests