[TRA-152] refresh all vault orders #1213
Conversation
WalkthroughThe updates involve enhancing the vault functionality within a blockchain protocol, introducing new constants, and refining the management of vault orders. A focus on testing ensures the reliability of these features, particularly through the addition of tests for endblocker functionality and order refresh mechanisms. Modifications in the keeper logic cater to different vault types and improve the handling of vault orders, showcasing a concerted effort to bolster the protocol's vault operations. 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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/lib/metrics/constants.go (1 hunks)
- protocol/x/vault/abci_test.go (1 hunks)
- protocol/x/vault/keeper/orders.go (5 hunks)
- protocol/x/vault/keeper/orders_test.go (3 hunks)
- protocol/x/vault/keeper/shares.go (2 hunks)
- protocol/x/vault/types/vault_id.go (2 hunks)
- protocol/x/vault/types/vault_id_test.go (2 hunks)
Additional comments: 10
protocol/x/vault/types/vault_id_test.go (1)
- 33-50: The addition of
TestToSubaccountIdeffectively tests the conversion of vault IDs to subaccount IDs using constants and assertions. This is a good practice for ensuring the correctness of theToSubaccountIdmethod.protocol/x/vault/keeper/shares.go (1)
- 44-49: The addition of the
getTotalSharesIteratorfunction follows best practices for iterating over key-value pairs in Cosmos SDK applications, including the correct use of a prefix store and the deferment of iterator closure. This is a solid implementation for retrieving an iterator over allTotalShares.protocol/x/vault/types/vault_id.go (1)
- 28-57: The additions in
vault_id.go, including theToSubaccountIdandIncrCounterWithLabelsmethods, are well-implemented. They follow best practices for constructing subaccount IDs from vault IDs and for enhancing monitoring capabilities with vault-specific metrics labels.protocol/x/vault/keeper/orders.go (3)
- 38-61: The introduction of the
RefreshAllVaultOrdersmethod is a significant addition that enhances the system's ability to maintain accurate and up-to-date vault orders. This method, along with its implementation details, follows best practices and adheres to the project's architectural patterns.- 63-107: The
RefreshVaultClobOrdersmethod adds specific functionality for refreshing CLOB vault orders. Its implementation, including error handling and metrics incrementing, is well-structured and adheres to best practices.- 151-157: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [154-183]
The update to the
GetVaultClobOrdersmethod to usevaultId.ToSubaccountId()simplifies the code and improves maintainability. This change, along with the detailed logic for placing new CLOB orders, is well-implemented.protocol/x/vault/keeper/orders_test.go (3)
- 27-139: The tests for
RefreshAllVaultOrdersare comprehensive, covering various scenarios and ensuring the functionality works as expected across different vault states. These tests are well-structured and follow best practices for unit testing.- 141-215: The tests for
RefreshVaultClobOrderseffectively validate the functionality for refreshing specific vault orders, including handling success cases and expected errors. The structure and assertions used in these tests adhere to best practices.- 24-219: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [216-361]
The adjustments in
TestGetVaultClobOrdersalign with the changes in the main codebase and ensure the tests remain relevant and accurate. The detailed scenarios tested here contribute to the robustness of the system's testing strategy.protocol/lib/metrics/constants.go (1)
- 246-250: The newly added constants related to vault functionality (
VaultCancelOrder,VaultPlaceOrder,VaultType, andVaultId) are correctly named and follow the existing naming convention. They are also appropriately placed in alphabetical order within their category.However, it's advisable to periodically review the entire list of constants for potential duplication or unused constants, ensuring the codebase remains clean and maintainable.
|
|
||
| import "testing" | ||
|
|
||
| // TODO (TRA-168): add endblocker test once deposit is implemented. |
There was a problem hiding this comment.
The TODO comment indicates that an endblocker test will be added once the deposit functionality is implemented. It's important to ensure that this TODO is tracked in a project management tool or issue tracker to ensure it's addressed in a timely manner.
Would you like me to open a GitHub issue to track this task?
|
|
||
| // Refresh orders depending on vault type. | ||
| switch vaultId.Type { | ||
| case types.VaultType_VAULT_TYPE_CLOB: |
There was a problem hiding this comment.
this is the only vault type we support currently right?
nit: add comment that no other vault types exist
There was a problem hiding this comment.
nit: Add a default case and error there.
| @@ -30,10 +32,79 @@ const ( | |||
| ) | |||
|
|
|||
| // RefreshAllVaultOrders refreshes all orders for all vaults by | |||
| // TODO(TRA-134) | |||
| // 1. Cancelling all existing orders. | |||
| // 2. Placing new orders. | |||
| func (k Keeper) RefreshAllVaultOrders(ctx sdk.Context) { | |||
There was a problem hiding this comment.
will it be a final PR at the end that ties this to app.go?
Where is this called, would this be in CommitTx or in PrepareProposal?
There was a problem hiding this comment.
It's already tied (see here). At the end of every block, this will be called
| if _, exists := k.clobKeeper.GetLongTermOrderPlacement(ctx, order.OrderId); exists { | ||
| err := k.clobKeeper.HandleMsgCancelOrder(ctx, clobtypes.NewMsgCancelOrderStateful( | ||
| order.OrderId, | ||
| uint32(ctx.BlockTime().Unix())+ORDER_EXPIRATION_SECONDS, |
There was a problem hiding this comment.
why do we want to wait 5 seconds instead of just cancelling this now?
There was a problem hiding this comment.
this cancels the order now (doesn't wait for 5 seconds). have to set this to be currentBlockTime + 5 for this cancellation message to be valid
| return err | ||
| } | ||
| for _, order := range ordersToPlace { | ||
| err := k.clobKeeper.HandleMsgPlaceOrder(ctx, clobtypes.NewMsgPlaceOrder(*order)) |
There was a problem hiding this comment.
Should we make the optimization if the order already exists, do not remove it? Would save us two operations
There was a problem hiding this comment.
every block, we want to refresh these orders so that their sizes and prices are updated
There was a problem hiding this comment.
if the oracle price doesn't update, and nobody has filled us, in the current strategy, all order prices and sizes should be the same.
I don't know how often the situation above happens, or how much faster we would run if we made this optimization, but there is definitely a scenario where our orders don't change between blocks.
There was a problem hiding this comment.
yep there can be. created a ticket here to work on optimization later
|
|
||
| // Check that there's no stateful orders yet. | ||
| allStatefulOrders := tApp.App.ClobKeeper.GetAllStatefulOrders(ctx) | ||
| require.Len(t, allStatefulOrders, 0) |
There was a problem hiding this comment.
nit: Should we test that this is cancelling existing orders?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/vault/keeper/orders.go (5 hunks)
- protocol/x/vault/keeper/orders_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
| return err | ||
| } | ||
| for _, order := range ordersToCancel { | ||
| if _, exists := k.clobKeeper.GetLongTermOrderPlacement(ctx, order.OrderId); exists { |
There was a problem hiding this comment.
Is this method able to find orders that are partially filled?
For example, if the vault had half of its order filled, it would still get canceled in RefreshVaultClobOrders, correct?
Changelist
refresh all vault orders
Test Plan
added 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.