[TRA-169] keep existing vault orders if price and size don't need updates #1886
Conversation
tqin7
changed the title
WalkthroughThe changes introduce a new field Changes
Poem
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 (
|
| clobmoduletypes.ModuleName, | ||
| sendingmoduletypes.ModuleName, | ||
| vestmoduletypes.ModuleName, | ||
| rewardsmoduletypes.ModuleName, | ||
| epochsmoduletypes.ModuleName, | ||
| govplusmoduletypes.ModuleName, | ||
| delaymsgmoduletypes.ModuleName, | ||
| vaultmoduletypes.ModuleName, |
There was a problem hiding this comment.
previously, vault endblocker was moved to be in front of clob to cancel orders before they expire to avoid flickering (as order expiration was set to be a very small value). With this PR, moving vault endblocker back as order expiration is significantly increased
There was a problem hiding this comment.
Why does the endblocker for the vault module need to be moved back?
There was a problem hiding this comment.
if vault endblocker is before clob's:
let's say an order is due for expiration, vault endblocker will run and see that the order still exists (thus don't do anything if price / size don't need updates), then clob endblocker will remove expired order. Then there's no order anymore until the next block's endblocker
There was a problem hiding this comment.
Note this behavior in a comment.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (14)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (6 hunks)
- proto/dydxprotocol/vault/genesis.proto (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/lib/bytes.go (2 hunks)
- protocol/lib/bytes_test.go (1 hunks)
- protocol/x/vault/genesis.go (1 hunks)
- protocol/x/vault/keeper/orders.go (4 hunks)
- protocol/x/vault/keeper/orders_test.go (12 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/types/keys.go (1 hunks)
- protocol/x/vault/types/orders.go (1 hunks)
- protocol/x/vault/types/orders_test.go (1 hunks)
- protocol/x/vault/types/params.go (1 hunks)
- protocol/x/vault/types/vault_id.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/x/vault/types/keys.go
Additional comments not posted (38)
protocol/x/vault/types/orders.go (1)
7-19: LGTM! Verify correctness with additional unit tests.The function correctly calculates the CLOB order client ID by combining the order side and layer into a 32-bit integer. Ensure thorough testing to verify correctness.
protocol/lib/bytes.go (2)
18-25: LGTM! Verify correctness with additional unit tests.The function correctly converts a slice of uint32 to a byte slice. Ensure thorough testing to verify correctness.
27-34: LGTM! Verify correctness with additional unit tests.The function correctly converts a byte slice to a slice of uint32. Ensure thorough testing to verify correctness.
proto/dydxprotocol/vault/genesis.proto (1)
28-29: LGTM! Verify handling of the new field in other parts of the code.The new field
most_recent_client_idscorrectly stores the client IDs of the most recently placed orders. Ensure thorough testing to verify the handling of this field in other parts of the code.protocol/x/vault/genesis.go (1)
36-36: LGTM! Verify implementation and testing ofSetMostRecentClientIds.The updates to the
InitGenesisfunction correctly set the most recent client IDs for each vault. Ensure thorough testing to verify the implementation and testing of theSetMostRecentClientIdsfunction.protocol/x/vault/types/params.go (1)
17-17: LGTM! Verify the impact of increasingOrderExpirationSeconds.The change from 2 to 10 seconds looks good. Ensure this change does not negatively impact other parts of the codebase.
protocol/lib/bytes_test.go (2)
28-57: Good test coverage forUint32ArrayToBytes.The test cases cover a range of scenarios, including edge cases.
59-88: Good test coverage forBytesToUint32Array.The test cases cover a range of scenarios, including edge cases.
protocol/x/vault/types/orders_test.go (1)
12-87: Good test coverage forGetVaultClobOrderClientId.The test cases cover a range of scenarios, including various combinations of
sideandlayer.protocol/x/vault/types/vault_id.go (1)
74-82: FunctionGetClobOrderIdlooks good.The function implementation looks correct and follows best practices.
protocol/x/vault/keeper/vault.go (2)
144-145: Include fetching most recent client IDs for vaults.The addition correctly fetches the most recent client IDs for vaults.
151-151: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
GetAllVaultscorrectly handle theMostRecentClientIdsfield.Verification successful
Verification Successful: Handling of
MostRecentClientIdsFieldThe
MostRecentClientIdsfield is correctly handled in theInitGenesisfunction whenGetAllVaultsis used to setgenesis.Vaults.
protocol/x/vault/genesis.go:
- Line 36:
k.SetMostRecentClientIds(ctx, *vault.VaultId, vault.MostRecentClientIds)Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `GetAllVaults` correctly handle the `MostRecentClientIds` field. # Test: Search for the function usage. Expect: Ensure the new field is handled properly in all occurrences. rg --type go -A 5 $'GetAllVaults'Length of output: 948
Script:
#!/bin/bash # Extract relevant code around the usage of GetAllVaults in protocol/x/vault/genesis.go to verify handling of MostRecentClientIds. rg --type go -A 10 -B 10 'k.GetAllVaults(ctx)' protocol/x/vault/genesis.goLength of output: 76
Script:
#!/bin/bash # Extract a larger context around the usage of GetAllVaults in protocol/x/vault/genesis.go to verify handling of MostRecentClientIds. rg --type go -A 20 -B 20 'k.GetAllVaults(ctx)' protocol/x/vault/genesis.go # Search for the usage of genesis.Vaults to verify how the returned vault objects are handled. rg --type go 'genesis.Vaults' protocol/x/vault/genesis.goLength of output: 172
Script:
#!/bin/bash # Extract the entire InitGenesis function from protocol/x/vault/genesis.go to verify handling of genesis.Vaults. ast-grep --lang go --pattern 'func InitGenesis($_, $_, $_) $_' protocol/x/vault/genesis.goLength of output: 1936
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (4)
37-39: AddmostRecentClientIdsfield toVaultinterface.The addition correctly introduces the new field.
55-57: Addmost_recent_client_idsfield toVaultSDKTypeinterface.The addition correctly introduces the new field.
120-121: InitializemostRecentClientIdsincreateBaseVaultfunction.The addition correctly initializes the new field.
143-149: EncodemostRecentClientIdsinVault.encodefunction.The addition correctly encodes the new field.
protocol/x/vault/keeper/orders.go (7)
77-79: Include fetching most recent client IDs inRefreshVaultClobOrders.The addition correctly fetches the most recent client IDs.
86-110: Include setting most recent client IDs inRefreshVaultClobOrders.The addition correctly sets the most recent client IDs.
386-401: AddinternalPlaceVaultClobOrderfunction.The new function correctly places a vault CLOB order internally.
404-425: UpdatePlaceVaultClobOrderto callinternalPlaceVaultClobOrder.The update correctly calls the new function to place a vault CLOB order.
427-467: AddReplaceVaultClobOrderfunction.The new function correctly replaces a vault CLOB order.
469-480: AddGetMostRecentClientIdsfunction.The new function correctly retrieves the most recent client IDs for a vault.
482-489: AddSetMostRecentClientIdsfunction.The new function correctly sets the most recent client IDs for a vault.
protocol/x/vault/keeper/orders_test.go (14)
6-6: Add missing import fortime.The import statement for
timeis added correctly.
16-16: Add missing import fortesttx.The import statement for
testtxis added correctly.
122-125: Initialize the test application correctly.The initialization of the test application with the given options is done correctly.
Line range hint
175-200:
Verify the correct placement of expected orders and indexer events.The code correctly checks the expected orders and indexer events. Ensure that the logic for appending orders and events is correct.
205-207: Check the final assertions for stateful orders and indexer events.The final assertions for stateful orders and indexer events are correctly implemented.
225-227: Define test case for refreshing orders from Vault for Clob Pair 1.The test case for refreshing orders from Vault for Clob Pair 1 is correctly defined.
239-243: Initialize the test application correctly.The initialization of the test application with the given options is done correctly.
295-307: Verify the initial orders and no order refresh logic.The code correctly verifies the initial orders and ensures that no order refresh occurs without price updates.
309-337: Verify the order replacement logic with price updates.The code correctly verifies the order replacement logic when price updates occur. Ensure that the logic for advancing the block and updating market prices is correct.
339-349: Verify the order placement logic after order expiration.The code correctly verifies the order placement logic after the orders expire.
831-831: Usevaulttypes.GetVaultClobOrderClientIdfor generating client IDs.The function
vaulttypes.GetVaultClobOrderClientIdis correctly used for generating client IDs.
925-931: Usevaulttypes.GetVaultClobOrderClientIdfor generating client IDs.The function
vaulttypes.GetVaultClobOrderClientIdis correctly used for generating client IDs.
925-931: Usevaulttypes.GetVaultClobOrderClientIdfor generating client IDs.The function
vaulttypes.GetVaultClobOrderClientIdis correctly used for generating client IDs.
950-974: Test the functionality of setting and getting the most recent client IDs.The test cases correctly verify the functionality of setting and getting the most recent client IDs.
protocol/app/app.go (1)
1298-1298: LGTM! The change in argument order is approved.The change in the order of arguments does not affect the logic or correctness of the code.
| @@ -72,78 +73,46 @@ func (k Keeper) RefreshAllVaultOrders(ctx sdk.Context) { | |||
| } | |||
|
|
|||
| // RefreshVaultClobOrders refreshes orders of a CLOB vault. | |||
| // Note: Order IDs are deterministically constructed based on layer and side. An order ID has its | |||
| // last bit flipped only upon order replacement. | |||
| func (k Keeper) RefreshVaultClobOrders(ctx sdk.Context, vaultId types.VaultId) (err error) { | |||
There was a problem hiding this comment.
main logic change is in this function
| }, | ||
| ) | ||
| newOrders = tApp.App.ClobKeeper.GetAllStatefulOrders(ctx) | ||
| require.Len(t, newOrders, int(params.Layers*2)) |
There was a problem hiding this comment.
Check that client IDs are different from before?
There was a problem hiding this comment.
Or check that the expiration timestamps on the orders are updated.
|
What happens when existing vault orders are filled or partially filled? Does the new order quantums change from the old order's quantums and trigger a replacement order? |
Yep. Replacement happens when price / size changes |
| k.GetIndexerEventManager().AddTxnEvent( | ||
| ctx, | ||
| indexerevents.SubtypeStatefulOrder, | ||
| indexerevents.StatefulOrderEventVersion, | ||
| indexer_manager.GetBytes( | ||
| indexerevents.NewLongTermOrderReplacementEvent( | ||
| *replacedOrderId, | ||
| *order, | ||
| ), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
note: For expired orders, this breaks the order flickering logic since orders will have the expire + place events sent for them.
There was a problem hiding this comment.
correct. seems okay to me as expiration occurs much less frequently than before.
however, one thing we can do might be to replace order (send replacement event) here (i.e. replace order if order gets filled or expires)
| tApp.App.ClobKeeper.GetAllStatefulOrders(ctx), | ||
| ) | ||
|
|
||
| // Advance to next block with price updates and vault should replace its old orders with new ones. |
There was a problem hiding this comment.
Update test with case for filled orders / change in quantums.
| blockHeight int64 | ||
| // layer. | ||
| layer uint8 | ||
| func TestGetSetMostRecentClientIds(t *testing.T) { |
There was a problem hiding this comment.
nit: Prefer testcase organization for this test rather than commented cases.
| } | ||
| clientIds[i] = orderToPlace.OrderId.ClientId |
There was a problem hiding this comment.
nit: Make it explicit that if the order is not replaced oldClientId is what' stored into the clientIds.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
protocol/x/vault/types/genesis.pb.gois excluded by!**/*.pb.go
Files selected for processing (17)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts (6 hunks)
- proto/dydxprotocol/vault/genesis.proto (1 hunks)
- protocol/app/app.go (1 hunks)
- protocol/app/testdata/default_genesis_state.json (1 hunks)
- protocol/lib/bytes.go (2 hunks)
- protocol/lib/bytes_test.go (1 hunks)
- protocol/scripts/genesis/sample_pregenesis.json (1 hunks)
- protocol/testutil/constants/genesis.go (1 hunks)
- protocol/x/vault/genesis.go (1 hunks)
- protocol/x/vault/keeper/orders.go (4 hunks)
- protocol/x/vault/keeper/orders_test.go (12 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/types/keys.go (1 hunks)
- protocol/x/vault/types/orders.go (1 hunks)
- protocol/x/vault/types/orders_test.go (1 hunks)
- protocol/x/vault/types/params.go (1 hunks)
- protocol/x/vault/types/vault_id.go (2 hunks)
Files skipped from review due to trivial changes (1)
- protocol/testutil/constants/genesis.go
Files skipped from review as they are similar to previous changes (11)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/genesis.ts
- proto/dydxprotocol/vault/genesis.proto
- protocol/app/app.go
- protocol/lib/bytes.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/keeper/vault.go
- protocol/x/vault/types/orders.go
- protocol/x/vault/types/orders_test.go
- protocol/x/vault/types/params.go
- protocol/x/vault/types/vault_id.go
Additional comments not posted (6)
protocol/x/vault/types/keys.go (1)
30-32: Addition of MostRecentClientIdsKeyPrefixThe new key prefix
MostRecentClientIdsKeyPrefixis added to store the most recent client IDs for vaults. The documentation comment is clear and follows the existing conventions.protocol/x/vault/genesis.go (1)
36-36: Setting Most Recent Client IDs in InitGenesisThe addition of
k.SetMostRecentClientIds(ctx, *vault.VaultId, vault.MostRecentClientIds)in theInitGenesisfunction is appropriate. It ensures that the most recent client IDs are set during the genesis initialization. Error handling is consistent with the rest of the code.protocol/lib/bytes_test.go (2)
28-57: Comprehensive Test for Uint32ArrayToBytesThe
TestUint32ArrayToBytesfunction is comprehensive and covers various edge cases, including maximum and minimum values. The test cases are well-structured and follow the existing testing conventions.
59-88: Comprehensive Test for BytesToUint32ArrayThe
TestBytesToUint32Arrayfunction is comprehensive and covers various edge cases, including maximum and minimum values. The test cases are well-structured and follow the existing testing conventions.protocol/app/testdata/default_genesis_state.json (1)
475-475: Update of order_expiration_seconds ParameterThe
order_expiration_secondsparameter is updated from2to60. This change is consistent with the PR objective to allow orders to stay longer before an update is needed.protocol/scripts/genesis/sample_pregenesis.json (1)
1823-1823: Updateorder_expiration_secondsto 60 seconds.The
order_expiration_secondsparameter has been updated to60to allow orders to stay as long as possible until an update is needed. This change aligns with the PR summary and the intended behavior described.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders_test.go (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/orders_test.go
| } | ||
| } | ||
|
|
||
| // Vault should place its initial orders. |
There was a problem hiding this comment.
nit: Add a check after each block that mostRecentClientIds of the vault is the same as all the stateful orders.
| ) | ||
| } | ||
|
|
||
| // Advance to next block where vault should replace its orders to update their sizes. |
There was a problem hiding this comment.
non-blocking: Add TODO for filled stateful vault order testing if possible.
| initialOrders := tApp.App.ClobKeeper.GetAllStatefulOrders(ctx) | ||
| require.Len(t, initialOrders, int(params.Layers*2)) | ||
|
|
||
| // Advance to next block with no price updates / order matches and vault should not refresh its orders. |
There was a problem hiding this comment.
nit: Advance a few more blocks while checking that orders do not get refreshed.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- protocol/x/vault/keeper/orders_test.go (12 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/orders_test.go
Changelist
this behavior is achieved by introducing a new store:
of the most recently placed orders (previously, we just calculate this with last block's height)
also set order expiration to be 60s into the future as a vault wants its order to stay as long as possible until an update is needed.
Test Plan
unit / integration tests
localnet testing
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