[IND-460] Emit deleveraging events from protocol #736
Conversation
WalkthroughThe changes introduced are primarily focused on adding support for deleveraging events in the dYdX protocol. This includes the addition of new interfaces, constants, handlers, validators, and test cases in the indexer service. The protocol has been updated with new functions and tests for creating and validating deleveraging events. Changes have also been made to the Changes
TipsChat with CodeRabbit Bot (
|
![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: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/indexer/events/events.pb.go
Files selected for processing (22)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- indexer/services/ender/tests/helpers/constants.ts (2 hunks)
- indexer/services/ender/tests/lib/on-message.test.ts (6 hunks)
- indexer/services/ender/tests/validators/deleveraging-validator.test.ts (1 hunks)
- indexer/services/ender/src/constants.ts (1 hunks)
- indexer/services/ender/src/handlers/deleveraging-handler.ts (1 hunks)
- indexer/services/ender/src/lib/block-processor.ts (2 hunks)
- indexer/services/ender/src/lib/helper.ts (2 hunks)
- indexer/services/ender/src/lib/types.ts (3 hunks)
- indexer/services/ender/src/validators/deleveraging-validator.ts (1 hunks)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
- protocol/indexer/events/constants.go (3 hunks)
- protocol/indexer/events/deleveraging.go (1 hunks)
- protocol/indexer/events/deleveraging_test.go (1 hunks)
- protocol/mocks/ClobKeeper.go (1 hunks)
- protocol/mocks/MemClob.go (1 hunks)
- protocol/x/clob/keeper/deleveraging.go (4 hunks)
- protocol/x/clob/keeper/liquidations.go (3 hunks)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
- protocol/x/clob/memclob/memclob.go (2 hunks)
- protocol/x/clob/types/liquidations_keeper.go (1 hunks)
- protocol/x/clob/types/memclob.go (1 hunks)
Files skipped from review due to trivial changes (7)
- indexer/services/ender/src/lib/block-processor.ts
- indexer/services/ender/src/lib/helper.ts
- indexer/services/ender/src/lib/types.ts
- indexer/services/ender/src/validators/deleveraging-validator.ts
- protocol/mocks/ClobKeeper.go
- protocol/mocks/MemClob.go
- protocol/x/clob/types/memclob.go
Additional comments: 32
indexer/services/ender/__tests__/validators/deleveraging-validator.test.ts (1)
- 1-80: The test cases are well written and cover both the positive and negative scenarios. The use of
it.eachfor testing different invalid cases is a good practice. ThecreateBlockfunction is a good abstraction for creating a mockIndexerTendermintBlockobject. The use ofjest.spyOnandjest.clearAllMocksinbeforeEachandafterEachrespectively ensures that the tests are isolated and do not affect each other.protocol/x/clob/keeper/liquidations.go (3)
6-7: Ensure that the newly imported packages are used in the code and that they don't introduce any breaking changes.
147-148: Ensure that the error returned by
MaybeDeleverageSubaccountis handled properly. If the error is not nil, the function should return immediately to prevent further execution.158-178: The loop that sends on-chain deleveraging events for each fill seems to be implemented correctly. However, ensure that the
GetIndexerEventManager().AddTxnEventfunction can handle the volume of events generated in this loop, especially if thefillsslice is large. Consider adding error handling for theAddTxnEventfunction if it can return an error.indexer/services/ender/src/constants.ts (1)
- 13-19: The new constant
DELEVERAGING_EVENT_TYPEis correctly defined and follows the existing pattern for event type constants. Ensure that this constant is used correctly in the event handling logic.proto/dydxprotocol/indexer/events/events.proto (1)
- 165-188: The new
DeleveragingEventV1message is well defined and includes all necessary fields to represent a deleveraging event. Ensure that the new message is correctly handled in all parts of the system where protobuf messages are processed.indexer/services/ender/__tests__/lib/on-message.test.ts (6)
21-24: The import of
DeleveragingEventV1is correct and necessary for the new test cases.43-46: The addition of
defaultDeleveragingEventis correct and necessary for the new test cases.54-59: The mocking of
DeleveragingHandleris correct and necessary for the new test cases.87-91: The mock return value for
DeleveragingHandleris correct and necessary for the new test cases.165-168: The creation of
defaultDeleveragingEventBinaryis correct and necessary for the new test cases.385-440: The new test case for processing a block with a deleveraging event is correct and necessary to ensure the new functionality works as expected. It checks that the
DeleveragingHandleris called with the correct arguments and that the correct stats are updated.protocol/indexer/events/deleveraging.go (1)
- 1-28: The function
NewDeleveragingEventseems to be well implemented. It takes in the necessary parameters, converts the subaccount IDs to indexer subaccount IDs, and returns a newDeleveragingEventV1struct. Ensure that the function is being used correctly throughout the codebase and that the parameters passed to it are valid and correctly formatted.protocol/x/clob/keeper/deleveraging.go (1)
- 27-41: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [27-112]
The function
MaybeDeleverageSubaccountnow returns a new slicefills []types.MatchPerpetualDeleveraging_Fillin addition to the existing return values. The return statement in the error case has been updated to returnnilfor the new slicefills. The return statement in the success case has been updated to return the new slicefills. The functionMaybeDeleverageSubaccountnow assigns the return value ofk.MemClob.DeleverageSubaccountto the new slicefills. Ensure that all calls to this function throughout the codebase have been updated to handle the new return value.protocol/x/clob/memclob/memclob.go (2)
- 695-701: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [698-702]
The function signature has been updated to return an additional value
fills []types.MatchPerpetualDeleveraging_Fill. Ensure that all calls to this function throughout the codebase have been updated to handle the new return value.
- 720-720: The function now returns
fills,quantumsDeleveraged, andnilforerr. Ensure that the error handling in the calling code is updated to handle the possibility ofnilerror.indexer/services/ender/__tests__/helpers/constants.ts (2)
26-34: The import of
DeleveragingEventV1from@dydxprotocol-indexer/v4-protosis new and is expected as per the PR summary.280-290: The
defaultDeleveragingEventconstant is new and is of typeDeleveragingEventV1. It is correctly initialized with the required properties.indexer/services/ender/src/handlers/deleveraging-handler.ts (3)
- 1-18: The
DeleveragingHandlerclass is currently a stub with no implementation. Ensure that theinternalHandlemethod is implemented to handle theDeleveragingEventV1events. Also, if there are any parallelization IDs that need to be returned by thegetParallelizationIdsmethod, they should be added. Otherwise, if parallelization is not required, you may want to add a comment explaining why the method returns an empty array.* 9-11: + // Returns an empty array as no parallelization is required return []; } * 14-17: + // TODO: Implement the handling of DeleveragingEventV1 events return []; }protocol/x/clob/types/liquidations_keeper.go (1)
- 23-29: The new function
MaybeDeleverageSubaccounthas been added to theLiquidationsKeeperinterface. Ensure that this function is implemented in all structs that implement this interface. Also, make sure to handle the returned error properly in all places where this function is called.protocol/indexer/events/deleveraging_test.go (1)
- 1-40: The test case
TestNewDeleveragingEvent_Successis well written and covers the success scenario for the functionNewDeleveragingEvent. It checks the correctness of the function by comparing the expected and actual results. However, it would be beneficial to add more test cases to cover edge cases and failure scenarios. This will ensure that the function behaves as expected in all scenarios and will help catch any potential bugs or issues.protocol/indexer/events/constants.go (3)
16-19: The new constant
SubtypeDeleveragingis introduced correctly. Ensure that it is used consistently across the codebase.34-35: The new event version
DeleveragingEventVersionis introduced correctly. Ensure that it is used consistently across the codebase.49-50: The new event subtype
SubtypeDeleveragingis added to theOnChainEventSubtypeslist correctly. Ensure that it is used consistently across the codebase.indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (6)
447-477: The
DeleveragingEventV1interface is well defined with clear comments explaining the purpose of each field. Ensure that the types used for each field are correct and consistent with the rest of the codebase.484-508: The
DeleveragingEventV1SDKTypeinterface is also well defined. Note that the field names are in snake_case, which is different from the camelCase used inDeleveragingEventV1. Ensure this is intentional and consistent with the rest of the codebase.1784-1793: The
createBaseDeleveragingEventV1function creates a baseDeleveragingEventV1object with default values. This is a good practice as it ensures that all fields in the object are initialized.1796-1821: The
encodefunction correctly encodes aDeleveragingEventV1object into a binary format. It checks if each field is defined before encoding it, which is a good practice.1824-1863: The
decodefunction correctly decodes a binary message into aDeleveragingEventV1object. It uses a switch statement to handle each field based on its tag, which is a good practice.1866-1874: The
fromPartialfunction correctly creates aDeleveragingEventV1object from a partial object. It checks if each field is defined in the partial object before assigning it, which is a good practice.
| clobPairId := k.mustGetClobPairForPerpetualId(ctx, perpetualId).Id | ||
| subticks := liquidationOrder.GetOrderSubticks() | ||
| isBuy := liquidationOrder.IsBuy() | ||
| // Send on-chain deleveraging event for each fill. | ||
| for _, fill := range fills { | ||
| k.GetIndexerEventManager().AddTxnEvent( | ||
| ctx, | ||
| indexerevents.SubtypeDeleveraging, | ||
| indexerevents.DeleveragingEventVersion, | ||
| indexer_manager.GetBytes( | ||
| indexerevents.NewDeleveragingEvent( | ||
| subaccountId, | ||
| fill.OffsettingSubaccountId, | ||
| clobPairId, | ||
| satypes.BaseQuantums(fill.FillAmount), | ||
| uint64(subticks), | ||
| isBuy, | ||
| ), | ||
| ), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Are there any tests we need to update to check that these new events are correctly emitted?
| isBuy := liquidationOrder.IsBuy() | ||
| // Send on-chain deleveraging event for each fill. | ||
| for _, fill := range fills { | ||
| k.GetIndexerEventManager().AddTxnEvent( |
There was a problem hiding this comment.
wait just to make sure we are on the same page - this is happening in PrepareCheckState / CheckState, so deleveraging fills are not on-chain yet. I don't think we should be sending an on-chain event?
deleveraging fills, similar to other order matches, get persisted to consensus state in DeliverTx here
probably easier to emit events in ProcessDeleveraging in Vincent's link.
There was a problem hiding this comment.
and for my understanding - CometBFT events are offchain events?
the function in your link is a shared function between check tx and deliver tx. so if we emit events there, we probably need an if statement and only emit for deliver tx?
There was a problem hiding this comment.
CometBFT events are not stored in consensus so yes they would be off-chain.
The indexer events are added to a TransientStore and only sent in the EndBlocker call back.
| // The ID of the clob pair that was liquidated. | ||
| uint32 clob_pair_id = 3; |
There was a problem hiding this comment.
nit: thought about this more. let's maybe use perpetual_id instead of clob_pair_id here.
CLOB actually assumes a one to many relationship between perp id and clob pair id (link). We currently only have 1 to 1 mapping, but using perpetual id is probably more resilient to changes in the future
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- protocol/indexer/events/events.pb.go
Files selected for processing (9)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (2 hunks)
- proto/dydxprotocol/indexer/events/events.proto (1 hunks)
- protocol/indexer/events/deleveraging.go (1 hunks)
- protocol/indexer/events/deleveraging_test.go (1 hunks)
- protocol/indexer/indexer_manager/event_manager.go (5 hunks)
- protocol/x/clob/keeper/deleveraging.go (2 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (7 hunks)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (1 hunks)
Files skipped from review due to trivial changes (3)
- protocol/indexer/indexer_manager/event_manager.go
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/keeper/process_operations_test.go
Additional comments: 14
proto/dydxprotocol/indexer/events/events.proto (1)
- 165-187: The new
DeleveragingEventV1message seems to be well defined and includes all necessary fields to describe a deleveraging event. Ensure that the new message is properly handled in the rest of the codebase.protocol/indexer/events/deleveraging.go (1)
- 1-28: The function
NewDeleveragingEventseems to be correctly implemented. It takes in the necessary parameters, converts the subaccount IDs to indexer subaccount IDs, and returns a pointer to the createdDeleveragingEventV1struct. Ensure that the function is being used correctly in the rest of the codebase and that the returned pointer is being handled properly to avoid null pointer dereferences.protocol/x/clob/keeper/deleveraging_test.go (7)
5-6: The new import statements are correctly added for the
indexereventsandindexer_managerpackages.615-616: The
mockIndexerEventManagervariable is correctly initialized and passed to theNewClobKeepersTestContextfunction.650-678: The event emission for each fill in the
TestOffsetSubaccountPerpetualPositionfunction is correctly implemented. Ensure that theAddTxnEventmethod of themockIndexerEventManageris correctly mocked in your tests.1037-1038: The
mockIndexerEventManagervariable is correctly initialized and passed to theNewClobKeepersTestContextfunction.1068-1091: The event emission for successful deleveraging in the
TestProcessDeleveragingfunction is correctly implemented. Ensure that theAddTxnEventmethod of themockIndexerEventManageris correctly mocked in your tests.1186-1187: The
mockIndexerEventManagervariable is correctly initialized and passed to theNewClobKeepersTestContextfunction.1225-1248: The event emission for successful deleveraging in the
TestProcessDeleveraging_Roundingfunction is correctly implemented. Ensure that theAddTxnEventmethod of themockIndexerEventManageris correctly mocked in your tests.indexer/packages/v4-protos/src/codegen/dydxprotocol/indexer/events/events.ts (4)
453-474: The
DeleveragingEventV1interface is well defined and includes all necessary fields for a deleveraging event. Ensure that the types used for each field are correct and consistent with the rest of the codebase.481-501: The
DeleveragingEventV1SDKTypeinterface is also well defined. Note that the field names are in snake_case, which is different from the camelCase used inDeleveragingEventV1. Ensure this is intentional and consistent with the rest of the codebase.1778-1787: The
createBaseDeleveragingEventV1function creates a baseDeleveragingEventV1object with default values. This is a good practice for creating base objects.1789-1871: The
DeleveragingEventV1object includesencode,decode, andfromPartialfunctions. These are necessary for handlingDeleveragingEventV1objects. Ensure that these functions are correctly implemented and tested.protocol/x/clob/keeper/deleveraging.go (1)
- 3-10: The new import statements are correctly placed and formatted. Ensure that these packages are included in your dependencies.
| package events_test | ||
|
|
||
| import ( | ||
| satypes "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types" | ||
| "testing" | ||
|
|
||
| "github.com/dydxprotocol/v4-chain/protocol/indexer/events" | ||
| v1 "github.com/dydxprotocol/v4-chain/protocol/indexer/protocol/v1" | ||
| "github.com/dydxprotocol/v4-chain/protocol/testutil/constants" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| var ( | ||
| liquidatedSubaccountId = constants.Alice_Num0 | ||
| offsettingSubaccountId = constants.Bob_Num0 | ||
| perpetualId = uint32(1) | ||
| price = satypes.BaseQuantums(1000) | ||
| isBuy = true | ||
| ) | ||
|
|
||
| func TestNewDeleveragingEvent_Success(t *testing.T) { | ||
| deleveragingEvent := events.NewDeleveragingEvent( | ||
| liquidatedSubaccountId, | ||
| offsettingSubaccountId, | ||
| perpetualId, | ||
| fillAmount, | ||
| price, | ||
| isBuy, | ||
| ) | ||
| indexerLiquidatedSubaccountId := v1.SubaccountIdToIndexerSubaccountId(liquidatedSubaccountId) | ||
| indexerOffsettingSubaccountId := v1.SubaccountIdToIndexerSubaccountId(offsettingSubaccountId) | ||
| expectedDeleveragingEventProto := &events.DeleveragingEventV1{ | ||
| Liquidated: indexerLiquidatedSubaccountId, | ||
| Offsetting: indexerOffsettingSubaccountId, | ||
| PerpetualId: perpetualId, | ||
| FillAmount: fillAmount.ToUint64(), | ||
| Price: price.ToUint64(), | ||
| IsBuy: isBuy, | ||
| } | ||
| require.Equal(t, expectedDeleveragingEventProto, deleveragingEvent) | ||
| } |
There was a problem hiding this comment.
The test case TestNewDeleveragingEvent_Success seems to be well written and covers the success scenario for the function NewDeleveragingEvent. It checks if the function correctly creates a DeleveragingEventV1 struct with the provided parameters. However, it would be beneficial to add more test cases to cover edge cases and failure scenarios. For example, what happens when invalid or null parameters are passed? This will help ensure the robustness of your code.
| ), | ||
| ) | ||
|
|
||
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | ||
| // if the branched state is discarded, so batching is not necessary. | ||
| k.GetIndexerEventManager().AddTxnEvent( | ||
| ctx, | ||
| indexerevents.SubtypeDeleveraging, | ||
| indexerevents.DeleveragingEventVersion, | ||
| indexer_manager.GetBytes( | ||
| indexerevents.NewDeleveragingEvent( | ||
| liquidatedSubaccountId, | ||
| offsettingSubaccountId, | ||
| perpetualId, | ||
| satypes.BaseQuantums(new(big.Int).Abs(deltaQuantums).Uint64()), | ||
| satypes.BaseQuantums(bankruptcyPriceQuoteQuantums.Uint64()), | ||
| deltaQuantums.Sign() > 0, | ||
| ), | ||
| ), | ||
| ) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The ProcessDeleveraging function is correctly implemented. It emits an event and sends an on-chain update for the deleveraging. However, it's important to ensure that the GetIndexerEventManager().AddTxnEvent() function handles errors correctly and that these errors are propagated up the call stack. If not, consider modifying the function to return an error and handle it here.
- k.GetIndexerEventManager().AddTxnEvent(
+ err = k.GetIndexerEventManager().AddTxnEvent(
ctx,
indexerevents.SubtypeDeleveraging,
indexerevents.DeleveragingEventVersion,
indexer_manager.GetBytes(
indexerevents.NewDeleveragingEvent(
liquidatedSubaccountId,
offsettingSubaccountId,
perpetualId,
satypes.BaseQuantums(new(big.Int).Abs(deltaQuantums).Uint64()),
satypes.BaseQuantums(bankruptcyPriceQuoteQuantums.Uint64()),
deltaQuantums.Sign() > 0,
),
),
)
+ if err != nil {
+ return err
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| ), | |
| ) | |
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |
| // if the branched state is discarded, so batching is not necessary. | |
| k.GetIndexerEventManager().AddTxnEvent( | |
| ctx, | |
| indexerevents.SubtypeDeleveraging, | |
| indexerevents.DeleveragingEventVersion, | |
| indexer_manager.GetBytes( | |
| indexerevents.NewDeleveragingEvent( | |
| liquidatedSubaccountId, | |
| offsettingSubaccountId, | |
| perpetualId, | |
| satypes.BaseQuantums(new(big.Int).Abs(deltaQuantums).Uint64()), | |
| satypes.BaseQuantums(bankruptcyPriceQuoteQuantums.Uint64()), | |
| deltaQuantums.Sign() > 0, | |
| ), | |
| ), | |
| ) | |
| return nil | |
| ), | |
| ) | |
| // Send on-chain update for the deleveraging. The events are stored in a TransientStore which should be rolled-back | |
| // if the branched state is discarded, so batching is not necessary. | |
| err = k.GetIndexerEventManager().AddTxnEvent( | |
| ctx, | |
| indexerevents.SubtypeDeleveraging, | |
| indexerevents.DeleveragingEventVersion, | |
| indexer_manager.GetBytes( | |
| indexerevents.NewDeleveragingEvent( | |
| liquidatedSubaccountId, | |
| offsettingSubaccountId, | |
| perpetualId, | |
| satypes.BaseQuantums(new(big.Int).Abs(deltaQuantums).Uint64()), | |
| satypes.BaseQuantums(bankruptcyPriceQuoteQuantums.Uint64()), | |
| deltaQuantums.Sign() > 0, | |
| ), | |
| ), | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| return nil |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/tests/helpers/constants.ts (2 hunks)
Additional comments: 2
indexer/services/ender/__tests__/helpers/constants.ts (2)
26-36: The import statements are well-organized and follow the best practice of grouping and separating external and internal imports.
280-290: The
defaultDeleveragingEventconstant is correctly defined and all required properties are included. Ensure that thedefaultSenderSubaccountIdanddefaultRecipientSubaccountIdare valid and correctly initialized before this point.
| mock.Anything, | ||
| indexerevents.SubtypeDeleveraging, | ||
| indexerevents.DeleveragingEventVersion, | ||
| mock.Anything, |
There was a problem hiding this comment.
nit: Is it possible to derive the deleveraging event here from the operation?
There was a problem hiding this comment.
Bankruptcy price isn't exposed by the API. I could add it to the test params, but that would require some more changes. Plus, the exact deleveraging event is being tested in multiple other e2e tests. So, I think it's reasonable to omit here.
There was a problem hiding this comment.
Sounds good, can we add a comment to that effect?
| @@ -2018,6 +2018,9 @@ func TestPlacePerpetualLiquidation_Deleveraging(t *testing.T) { | |||
| } | |||
|
|
|||
| if tc.expectedFilledSize == 0 { | |||
| mockIndexerEventManager.On("AddTxnEvent", | |||
| mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, | |||
There was a problem hiding this comment.
nit: Can the actual deleveraging event be derived here?
There was a problem hiding this comment.
Bankruptcy price isn't exposed by the API. I could add it to the test params, but that would require some more changes. Plus, the exact deleveraging event is being tested in multiple other e2e tests. So, I think it's reasonable to omit here.
There was a problem hiding this comment.
Can you add comment with the above details?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/clob/keeper/liquidations_test.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (1 hunks)
Files skipped from review due to trivial changes (2)
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/keeper/process_operations_test.go
Changelist
Emit deleveraging on-chain events from protocol
Add skeleton for Ender handling/validation
Test Plan
Unit tests
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.