Add migration function for stateful order expirations #1890
Conversation
WalkthroughThe changes introduce functions for migrating order expiration state from time-based slices to individual keys within the protocol's clob keeper. These updates ensure better state management and include improvements like closing iterators to avoid resource leaks. Additionally, tests have been introduced to validate these migrations, ensuring that the new functionality works correctly under various scenarios. 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 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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/stateful_order_state.go (2 hunks)
- protocol/x/clob/keeper/stateful_order_state_test.go (1 hunks)
Additional comments not posted (4)
protocol/x/clob/keeper/stateful_order_state.go (3)
233-233: Approved: Proper resource management withdefer it.Close().The addition of
defer it.Close()ensures that the iterator is properly closed, preventing potential resource leaks.
459-478: Approved: New migration functionMigrateOrderExpirationState.The
MigrateOrderExpirationStatefunction correctly migrates order expiration state from time slices to individual keys. Proper resource management and error handling are included.
480-503: Approved: New deprecated functionLegacySetStatefulOrdersTimeSliceInState.The
LegacySetStatefulOrdersTimeSliceInStatefunction correctly sets sorted order IDs in the legacy format. This function is intended for testing the migration function.protocol/x/clob/keeper/stateful_order_state_test.go (1)
1076-1130: Approved: New test functionTestMigrateOrderExpirationState.The
TestMigrateOrderExpirationStatefunction correctly verifies the migration of order expiration state. It sets up different time slices, performs the migration, and ensures the expected order IDs match the actual ones. The test also ensures the old store is empty after migration.
roy-dydx
force-pushed
the
roy/statefulexpmigration
branch
from
7d01d55
to
29acd69
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
protocol/x/clob/keeper/stateful_order_state_test.go (1)
1076-1076: Add a function-level comment.Add a comment describing the purpose of this test function.
// TestMigrateOrderExpirationState verifies the migration of order expiration states.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/stateful_order_state.go (2 hunks)
- protocol/x/clob/keeper/stateful_order_state_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/stateful_order_state.go
Additional comments not posted (2)
protocol/x/clob/keeper/stateful_order_state_test.go (2)
1103-1103: Check the context initialization.Ensure that
keepertest.NewClobKeepersTestContextproperly initializes the context for the test.
1109-1109: Ensure successful migration.Verify that
MigrateOrderExpirationStatesuccessfully migrates the order expiration states.
| it := oldStore.Iterator(nil, nil) | ||
| require.False(t, it.Valid()) |
There was a problem hiding this comment.
Close the iterator.
Ensure that the iterator is properly closed to prevent resource leaks.
- require.False(t, it.Valid())
+ require.False(t, it.Valid())
+ it.Close()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.
| it := oldStore.Iterator(nil, nil) | |
| require.False(t, it.Valid()) | |
| it := oldStore.Iterator(nil, nil) | |
| require.False(t, it.Valid()) | |
| it.Close() |
There was a problem hiding this comment.
Nit: Close the iterator as suggested
There was a problem hiding this comment.
@teddyding, thanks for confirming the change to close the iterator.
| require.False(t, it.Valid()) | ||
|
|
||
| for goodTilTime, expectedOrderIds := range tc.timeSlicesToOrderIds { | ||
| orderIds := ks.ClobKeeper.GetStatefulOrderIdExpirations(ks.Ctx, goodTilTime) |
There was a problem hiding this comment.
For my understanding, what's the ordering on this returned orderId list?
There was a problem hiding this comment.
In sorted key order since it's done by store iteration. Ends up being sorted by serialized order id which can get weird since it's a serialized proto. But order doesn't matter for us here and we plan to migrate orderId.ToStateKey() in the future.
roy-dydx
force-pushed
the
roy/statefulexpmigration
branch
from
29acd69
to
c88bbc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/stateful_order_state.go (2 hunks)
- protocol/x/clob/keeper/stateful_order_state_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/stateful_order_state_test.go
Additional comments not posted (2)
protocol/x/clob/keeper/stateful_order_state.go (2)
233-233: Good practice: Properly closing the iterator.The addition of
defer it.Close()ensures that the iterator is properly closed, preventing potential resource leaks. This is a good practice.
484-507: Approved: Deprecated function for testing migration.The
LegacySetStatefulOrdersTimeSliceInStatefunction correctly sorts the order IDs before writing them to state and is appropriately marked as deprecated for testing purposes.
| // MigrateOrderExpirationState migrates order expiration state from slices based on time to individual keys. | ||
| // Deprecated: Only intended for use in the v5.2 upgrade handler. | ||
| func (k Keeper) MigrateOrderExpirationState(ctx sdk.Context) { | ||
| store := ctx.KVStore(k.storeKey) | ||
|
|
||
| prefixStore := prefix.NewStore( | ||
| store, | ||
| []byte(types.LegacyStatefulOrdersTimeSlicePrefix), //nolint:staticcheck | ||
| ) | ||
| it := prefixStore.Iterator(nil, nil) | ||
| defer it.Close() | ||
| for ; it.Valid(); it.Next() { | ||
| time, err := sdk.ParseTimeBytes(it.Key()) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key())) | ||
| } | ||
| var orders types.StatefulOrderTimeSliceValue | ||
| k.cdc.MustUnmarshal(it.Value(), &orders) | ||
| for _, orderId := range orders.OrderIds { | ||
| k.AddStatefulOrderIdExpiration(ctx, time, orderId) | ||
| } | ||
| prefixStore.Delete(it.Key()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Approved with a suggestion: Handle panics for better traceability.
The MigrateOrderExpirationState function properly migrates order expiration state and ensures data integrity by panicking on malformed times. However, for better traceability, consider catching the panic and logging the error before re-panicking.
for ; it.Valid(); it.Next() {
time, err := sdk.ParseTimeBytes(it.Key())
if err != nil {
log.Error("Migration failed due to malformed time", "time", it.Key(), "error", err)
panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key()))
}
// existing code
}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/stateful_order_state.go (2 hunks)
- protocol/x/clob/keeper/stateful_order_state_test.go (1 hunks)
Additional comments not posted (2)
protocol/x/clob/keeper/stateful_order_state.go (1)
485-508: LGTM!The function correctly sorts the order IDs before storing them in state.
protocol/x/clob/keeper/stateful_order_state_test.go (1)
1076-1131: LGTM!The test function comprehensively verifies the migration of order expiration state.
| // UnsafeMigrateOrderExpirationState migrates order expiration state from slices based on time to | ||
| // individual keys. | ||
| // Deprecated: Only intended for use in the v5.2 upgrade handler. | ||
| func (k Keeper) UnsafeMigrateOrderExpirationState(ctx sdk.Context) { | ||
| store := ctx.KVStore(k.storeKey) | ||
|
|
||
| prefixStore := prefix.NewStore( | ||
| store, | ||
| []byte(types.LegacyStatefulOrdersTimeSlicePrefix), //nolint:staticcheck | ||
| ) | ||
| it := prefixStore.Iterator(nil, nil) | ||
| defer it.Close() | ||
| for ; it.Valid(); it.Next() { | ||
| time, err := sdk.ParseTimeBytes(it.Key()) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key())) | ||
| } | ||
| var orders types.StatefulOrderTimeSliceValue | ||
| k.cdc.MustUnmarshal(it.Value(), &orders) | ||
| for _, orderId := range orders.OrderIds { | ||
| k.AddStatefulOrderIdExpiration(ctx, time, orderId) | ||
| } | ||
| prefixStore.Delete(it.Key()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add logging before panicking for better traceability.
Consider logging the error before panicking to improve traceability during migration.
for ; it.Valid(); it.Next() {
time, err := sdk.ParseTimeBytes(it.Key())
if err != nil {
log.Error("Migration failed due to malformed time", "time", it.Key(), "error", err)
panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key()))
}
// existing code
}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- protocol/x/clob/keeper/migration.go (1 hunks)
- protocol/x/clob/keeper/migration_test.go (1 hunks)
- protocol/x/clob/keeper/stateful_order_state.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/stateful_order_state.go
Additional comments not posted (3)
protocol/x/clob/keeper/migration.go (2)
12-36: LGTM! The function is well-structured and handles errors appropriately.The
UnsafeMigrateOrderExpirationStatefunction migrates order expiration state from slices to individual keys. The use of adeferstatement ensures the iterator is closed properly, and error handling is appropriately managed.
38-61: LGTM! The function is well-structured and ensures data integrity.The
LegacySetStatefulOrdersTimeSliceInStatefunction sorts the order IDs and ensures there are no duplicates before storing them. The function is marked as deprecated and is only intended for testing purposes.protocol/x/clob/keeper/migration_test.go (1)
16-71: LGTM! The test function is well-structured and covers different scenarios.The
TestUnsafeMigrateOrderExpirationStatefunction sets up the initial state, runs the migration, and verifies the results. It covers different scenarios, including multiple time slices and no time slices, ensuring comprehensive test coverage.
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/x/clob/keeper/stateful_order_expiration_migration.go (1 hunks)
- protocol/x/clob/keeper/stateful_order_expiration_migration_test.go (1 hunks)
Additional comments not posted (5)
protocol/x/clob/keeper/stateful_order_expiration_migration.go (2)
15-35: LGTM!The function is well-structured and closes the iterator to avoid resource leaks.
41-61: LGTM!The function is well-structured and correctly performs the necessary operations.
protocol/x/clob/keeper/stateful_order_expiration_migration_test.go (3)
20-34: LGTM!The test case is comprehensive and correctly validates the migration functionality with multiple time slices.
35-37: LGTM!The test case is comprehensive and correctly validates the migration functionality with no time slices.
16-70: LGTM!The test cases are well-structured and correctly validate the migration functionality.
| func (k Keeper) UnsafeMigrateOrderExpirationState(ctx sdk.Context) { | ||
| store := ctx.KVStore(k.storeKey) | ||
|
|
||
| prefixStore := prefix.NewStore( | ||
| store, | ||
| []byte(types.LegacyStatefulOrdersTimeSlicePrefix), //nolint:staticcheck | ||
| ) | ||
| it := prefixStore.Iterator(nil, nil) | ||
| defer it.Close() | ||
| for ; it.Valid(); it.Next() { | ||
| time, err := sdk.ParseTimeBytes(it.Key()) | ||
| if err != nil { | ||
| panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key())) | ||
| } | ||
| var orders types.StatefulOrderTimeSliceValue | ||
| k.cdc.MustUnmarshal(it.Value(), &orders) | ||
| for _, orderId := range orders.OrderIds { | ||
| k.AddStatefulOrderIdExpiration(ctx, time, orderId) | ||
| } | ||
| prefixStore.Delete(it.Key()) | ||
| } |
There was a problem hiding this comment.
Consider replacing panic with proper error handling.
Using panic for error handling can cause the entire application to terminate abruptly. Consider returning an error or using a more graceful error-handling mechanism.
- if err != nil {
- panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key()))
- }
+ if err != nil {
+ return fmt.Errorf("migration failed due to malformed time: %s", it.Key())
+ }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.
| func (k Keeper) UnsafeMigrateOrderExpirationState(ctx sdk.Context) { | |
| store := ctx.KVStore(k.storeKey) | |
| prefixStore := prefix.NewStore( | |
| store, | |
| []byte(types.LegacyStatefulOrdersTimeSlicePrefix), //nolint:staticcheck | |
| ) | |
| it := prefixStore.Iterator(nil, nil) | |
| defer it.Close() | |
| for ; it.Valid(); it.Next() { | |
| time, err := sdk.ParseTimeBytes(it.Key()) | |
| if err != nil { | |
| panic(fmt.Sprintf("migration failed due to malformed time: %s", it.Key())) | |
| } | |
| var orders types.StatefulOrderTimeSliceValue | |
| k.cdc.MustUnmarshal(it.Value(), &orders) | |
| for _, orderId := range orders.OrderIds { | |
| k.AddStatefulOrderIdExpiration(ctx, time, orderId) | |
| } | |
| prefixStore.Delete(it.Key()) | |
| } | |
| func (k Keeper) UnsafeMigrateOrderExpirationState(ctx sdk.Context) error { | |
| store := ctx.KVStore(k.storeKey) | |
| prefixStore := prefix.NewStore( | |
| store, | |
| []byte(types.LegacyStatefulOrdersTimeSlicePrefix), //nolint:staticcheck | |
| ) | |
| it := prefixStore.Iterator(nil, nil) | |
| defer it.Close() | |
| for ; it.Valid(); it.Next() { | |
| time, err := sdk.ParseTimeBytes(it.Key()) | |
| if err != nil { | |
| return fmt.Errorf("migration failed due to malformed time: %s", it.Key()) | |
| } | |
| var orders types.StatefulOrderTimeSliceValue | |
| k.cdc.MustUnmarshal(it.Value(), &orders) | |
| for _, orderId := range orders.OrderIds { | |
| k.AddStatefulOrderIdExpiration(ctx, time, orderId) | |
| } | |
| prefixStore.Delete(it.Key()) | |
| } | |
| return nil | |
| } |
Co-authored-by: Tian Qin <tian@dydx.exchange> (cherry picked from commit 6da7590)
#1945) Co-authored-by: roy-dydx <133032749+roy-dydx@users.noreply.github.com>
Changelist
Test Plan
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
RemoveExpiredStatefulOrdersfunction to prevent resource leaks.Tests