[TRA-139] Add logic to transfer collateral when open/close isolated perpetual position. #1200
Conversation
…erpetual position.
WalkthroughThe recent updates introduce and refine the management of isolated perpetual positions within a financial protocol. A significant focus is on handling state transitions (opening, closing, etc.) for these positions, alongside mechanisms for transferring collateral accordingly. The introduction of new types and functions, alongside modifications to existing ones, aims to enhance the clarity, consistency, and functionality of the codebase. Additionally, a comprehensive test suite ensures the robustness of these updates, covering a wide range of scenarios for state transitions and collateral management. 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: 7
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (13 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (5 hunks)
Additional comments: 2
protocol/x/subaccounts/keeper/isolated_subaccount.go (1)
- 233-300: The
transferCollateralForIsolatedPerpetualfunction modifies thex/bankstate and handles collateral transfers based on state transitions. Ensure that error handling and rollback mechanisms are in place for transactions that modify state across modules (x/bankandx/subaccounts). This is crucial for maintaining data integrity in case of failures during the transfer process.protocol/x/subaccounts/keeper/subaccount.go (1)
- 556-577: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [535-714]
The
internalCanUpdateSubaccountsfunction, spanning lines 535 to 714, is responsible for validating updates to subaccounts and computing isolated perpetual position state transitions. This function is a critical component of the update process, and several aspects need to be highlighted:
Complexity and Readability: The function is lengthy and handles multiple responsibilities, including validation of updates, checking for undercollateralized subaccounts, and computing state transitions. Consider refactoring to improve readability and maintainability. Breaking down the function into smaller, more focused functions could enhance clarity and testability.
Error Handling: The function returns early in several places if an error is encountered. Ensure that all errors are handled appropriately and that any necessary cleanup or rollback actions are taken to maintain system integrity.
Validation Logic: The function includes complex logic for validating state transitions, especially for undercollateralized subaccounts. Ensure that this logic is thoroughly tested to prevent unintended behavior. Consider edge cases and ensure that the validation logic aligns with the protocol's rules and constraints.
Performance Considerations: The function iterates over updates and performs various checks and calculations. Evaluate the performance impact of these operations, especially in scenarios with a large number of updates. Optimize where possible to reduce computational overhead.
Documentation: Given the complexity of the function, detailed comments explaining the logic and rationale behind each major step would be beneficial. This would aid future maintainability and understanding of the code.
- // internalCanUpdateSubaccounts will validate all `updates` to the relevant subaccounts and compute - // if any of the updates led to an isolated perpetual position being opened or closed. + // internalCanUpdateSubaccounts validates all `updates` to the relevant subaccounts and computes + // isolated perpetual position state transitions. It checks for undercollateralized subaccounts, + // validates state transitions, and ensures updates adhere to protocol rules. Consider refactoring + // for improved readability, optimizing performance, and enhancing error handling. Detailed + // documentation is essential for complex logic sections.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (13 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount.go
Additional comments: 3
protocol/x/subaccounts/keeper/isolated_subaccount.go (3)
- 39-126: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [32-78]
The function
checkIsolatedSubaccountConstraintsis comprehensive and appears to correctly implement the logic for validating updates against isolated subaccount constraints. However, the complexity and length of this function suggest that it could benefit from further decomposition or refactoring to improve readability and maintainability.Consider breaking down this function into smaller, more focused functions that handle specific parts of the validation process.
- 39-126: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [81-185]
The function
processIsolatedPerpetualUpdateseffectively checks for constraint violations and computes state changes for isolated perpetual positions. However, there are several areas where the code could be optimized or clarified:
- The use of
math.MaxUint32as a default value forisolatedUpdatePerpetualIdandisolatedPositionPerpetualIdcould be replaced with a more explicit sentinel value or approach to improve readability.- The repeated checks for
hasIsolatedUpdateand conditions related to isolated and non-isolated perpetuals could be simplified or abstracted into helper functions.Consider refactoring to simplify the logic and improve code readability. Introduce helper functions where repetitive checks or operations occur.
- 233-300: The function
transferCollateralForIsolatedPerpetualhandles the collateral transfer logic based on the state transition of an isolated perpetual position. The function is well-structured and makes appropriate use of thex/bankkeeper. However, the repeated TODOs regarding non-USDC assets support (lines 258 and 281) indicate a significant area for future development.While the current implementation is focused on USDC, considering a more generic approach to asset handling could significantly enhance the protocol's adaptability and future-proofing.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount_test.go
Additional comments: 4
protocol/x/subaccounts/keeper/isolated_subaccount.go (4)
- 15-34: The implementation of the
positionStateTransitiontype and its associated methods is clear and follows best practices for defining and using enums in Go. The methodString()correctly handles unexpected state transitions, which is good for error handling and debugging.- 53-140: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [46-92]
The function
checkIsolatedSubaccountConstraintsis comprehensive and well-documented, effectively validating updates against isolated subaccount constraints and computing state transitions. It's good to see thorough error handling and clear separation of concerns within the function. However, ensure that the complexity of this function remains manageable as the protocol evolves, possibly by refactoring into smaller, more focused functions if necessary.
- 53-140: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [95-199]
The function
processIsolatedPerpetualUpdatesis logically sound and well-implemented, with clear handling of various constraints related to isolated perpetual positions. The function's documentation is thorough, aiding in understanding its purpose and behavior. Given the complexity of the constraints being checked, consider adding more detailed comments within the function body to explain the rationale behind specific checks, especially for future maintainers or contributors who may not be as familiar with the domain-specific logic.+ // Detailed comments explaining the rationale behind specific checks could enhance maintainability.
- 247-332: The function
transferCollateralForIsolatedPerpetualis well-implemented, with clear logic for handling collateral transfers based on state transitions of isolated perpetual positions. The use of thex/bankkeeper to modify state is correctly handled, and error handling is thorough. The TODO comment on line 316 about supporting non-USDC assets is consistent with previous observations, reinforcing the need to address this limitation in future updates.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (4 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (13 hunks)
- protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go (1 hunks)
- protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount.go
Additional comments: 9
protocol/x/subaccounts/types/isolated_perpetual_position_state_transition_test.go (1)
- 10-36: The test function
TestPositionStateTransitionStringis well-implemented, covering all expected outcomes of theStringmethod forPositionStateTransition. Good use of table-driven tests and assertions.protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go (3)
- 5-10: The
PositionStateTransitiontype and its constants are correctly defined and follow best practices for enum-like types in Go.- 17-24: The
Stringmethod forPositionStateTransitionis correctly implemented, including handling of unexpected values.- 26-34: The
IsolatedPerpetualPositionStateTransitionstruct is well-defined. However, there's a TODO comment about supporting non-USDC assets, indicating a limitation that should be addressed in future updates.protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1)
- 13-106: The test function
TestGetIsolatedPerpetualStateTransitionis well-implemented, covering various scenarios for determining state transitions for isolated perpetual positions. Good use of table-driven tests and assertions.protocol/x/subaccounts/keeper/isolated_subaccount.go (4)
- 23-113: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [15-65]
The
processIsolatedSubaccountUpdatesfunction is well-implemented, with clear documentation and thorough handling of isolated subaccount updates. Consider adding more detailed comments within complex logic sections to enhance maintainability.
- 23-113: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-173]
The
processSingleIsolatedSubaccountUpdatefunction is comprehensive and correctly implements the logic for checking constraints and computing state changes for isolated perpetuals. Consider adding unit tests specifically for this function to ensure all edge cases are covered.
- 175-216: The
GetIsolatedPerpetualStateTransitionfunction is correctly implemented, effectively computing state transitions for isolated perpetual positions. However, there's a TODO comment about supporting non-USDC assets, indicating a limitation that should be addressed in future updates.- 218-303: The
transferCollateralForIsolatedPerpetualfunction is well-implemented, correctly handling collateral transfers based on state transitions for isolated perpetual positions. Consider adding more detailed comments within complex logic sections to enhance maintainability.
| perptypes "github.com/dydxprotocol/v4-chain/protocol/x/perpetuals/types" | ||
| "github.com/dydxprotocol/v4-chain/protocol/x/subaccounts/types" | ||
| ) | ||
|
|
||
| // checkIsolatedSubaccountConstaints will validate all `updates` to the relevant subaccounts against | ||
| // isolated subaccount constraints. | ||
| // processIsolatedSubaccountUpdates will validate all `updates` to the relevant subaccounts against |
There was a problem hiding this comment.
Why change the name to "process" instead?
We're still only checking constraints and not doing any writes here?
There was a problem hiding this comment.
There's computations done as well to return the state transitions, so "processing" is done, and it's not just checks. No strong opinions, but having the same verb across both functions (inner and outer) is something I prefer.
| @@ -281,12 +284,13 @@ func (k Keeper) UpdateSubaccounts( | |||
| } | |||
|
|
|||
| allPerps := k.perpetualsKeeper.GetAllPerpetuals(ctx) | |||
| success, successPerUpdate, err = k.internalCanUpdateSubaccounts( | |||
| success, successPerUpdate, isolatedPerpetualPositionStateTransitions, err := k.internalCanUpdateSubaccounts( | |||
There was a problem hiding this comment.
Do we really need to store the state transitions and get them from this func?
AFAIU, the 2 checks are
- opening the first position
- closing the last position
Can we just add this after the constraints check since those 2 are separate and avoid creating the whole state transition object?
There was a problem hiding this comment.
That was my initial thought as well, however it led to a lot of repeated logic for finding the perpetual updates / perpetual positions that are isolated so it was slightly cleaner to pass them through/back the functions that check constraints as both flows have very similar logic.
Also not strongly opinionated on this, as a separate function also de-couples the 2 operations (however right now they are still tightly coupled in concept).
There was a problem hiding this comment.
My preference would be to decouple the 2 operations since and err towards the simpler logic.
How about just putting this in UpdatePerpetualPositions? You have all the information there and the collateral transfers are more closely tied to the actual perpetual update so it might make more sense here
There was a problem hiding this comment.
I still would need some data stored that gives enough information to do the collateral transfers in the form of the state transitions, and wouldn't want the collateral transfer logic to be in the same state update function.
UpdatePerpetualPositions could be a decent candidate for the computation, however to calculate the data for the resulting collateral transfer the AssetUpdates need to be unsettled, so I'd prefer doing this in the "checking" / "processing" functions before any updates to state happen and separate the state change functions from the functions calculating what state changes are needed.
There was a problem hiding this comment.
Moved the logic into post-update, and updated functions to use the post-updated subaccount + updates to figure out if positions were opened / closed.
Kept separation of logic to determine what state transition happened from what collateral transfer to do with x/bank.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/testutil/constants/positions.go (1 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (5 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (11 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (4 hunks)
- protocol/x/subaccounts/keeper/update.go (1 hunks)
- protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/subaccounts/keeper/isolated_subaccount_test.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/types/isolated_perpetual_position_state_transition.go
Additional comments: 5
protocol/x/subaccounts/keeper/update.go (1)
- 7-11: Renaming
settledUpdatetoSettledUpdatemakes the struct exported, which is a standard Go convention for types intended to be accessible outside the package. Ensure that all references to this struct across the codebase have been updated accordingly.Verification successful
The renaming of
settledUpdatetoSettledUpdateand its usage across the codebase is consistent and correctly implemented. All references to this struct have been appropriately updated to reflect its new, exported status.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to SettledUpdate are consistent across the codebase. rg --type go "SettledUpdate"Length of output: 3229
protocol/testutil/constants/positions.go (1)
- 79-79: The
Quantumsvalue forPerpetualPosition_OneISOLonghas been updated. Ensure that tests using this constant are reviewed and updated as necessary to reflect the new value and its implications on test scenarios.protocol/x/subaccounts/keeper/subaccount_helper.go (1)
- 15-15: Renaming the parameter from
settledUpdatetoSettledUpdatealigns with the updated struct name, enhancing code consistency. Ensure that all references to this parameter across the codebase have been updated accordingly.Verification successful
The script output suggests that the renaming of the parameter from
settledUpdatetoSettledUpdatehas been applied consistently across the codebase, with references found in multiple files and contexts. This aligns with the goal of enhancing code consistency. However, it's important to note that without inspecting each change in detail, there's a slight limitation in confirming the correctness of every reference contextually. Based on the available evidence, it appears that the review comment has been addressed correctly.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all references to SettledUpdate as a parameter are consistent across the codebase. rg --type go "SettledUpdate"Length of output: 3229
protocol/x/subaccounts/keeper/isolated_subaccount.go (2)
- 142-236: Introduced
GetIsolatedPerpetualStateTransitionfunction computes state transitions for isolated perpetual positions. This function is well-structured and follows good practices in handling various scenarios. Ensure that comprehensive unit tests cover all possible paths, especially edge cases around the number of perpetual updates and positions.- 238-318: The
transferCollateralForIsolatedPerpetualfunction manages collateral transfers between collateral pools. It's crucial to ensure robust error handling and logging for the collateral transfer process, especially given its impact on financial transactions. Consider adding more detailed logging for each step and potential failure point within this function to aid in debugging and operational monitoring.
| if stateTransition.Transition == types.Opened { | ||
| toModuleAddr = isolatedCollateralPoolAddr | ||
| fromModuleAddr = types.ModuleAddress | ||
| // If the isolated perpetual position was closed, then move collateral from the isolated |
There was a problem hiding this comment.
That's from go fmt, this indent is expected. I tried to fix it too and go fmt kept on changing it back.
| } | ||
|
|
||
| // If there are zero quantums to transfer, don't transfer collateral. | ||
| if stateTransition.QuoteQuantums.Sign() == 0 { |
There was a problem hiding this comment.
nit: maybe this should be at the top of the func?
| } | ||
|
|
||
| // Transfer collateral between collateral pools. | ||
| _, coinToTransfer, err := k.assetsKeeper.ConvertAssetToCoin( |
There was a problem hiding this comment.
informational q: what exactly are we converting for the collateral here?
There was a problem hiding this comment.
Converts between quantums (units that x/subaccounts use to track balances) and coins (units that x/bank use to track balances).
| // Transfer collateral between collateral pools for any isolated perpetual positions that changed | ||
| // state due to an update. | ||
| for _, settledUpdateWithUpdatedSubaccount := range settledUpdates { | ||
| // The subaccount in `settledUpdateWithUpdatedSubaccount` already has the perpetual updates | ||
| // and asset updates applied to it. | ||
| stateTransition, err := GetIsolatedPerpetualStateTransition( | ||
| settledUpdateWithUpdatedSubaccount, | ||
| allPerps, | ||
| ) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
| if err := k.transferCollateralForIsolatedPerpetual( | ||
| ctx, | ||
| stateTransition, | ||
| ); err != nil { | ||
| return false, nil, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: Make this into a helper function
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPositionStateTransitionString(t *testing.T) { |
There was a problem hiding this comment.
nit: Is this test necessary? Seems pretty trivial logic
There was a problem hiding this comment.
Not necessary, but could be useful if increase / decrease of position is a future state to be tracked.
|
E2E tests do have linear tickets already, noted in the PR already. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (5 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (11 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/isolated_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
Changelist
Update
UpdateSubaccountsto transfer collateral between collateral pools when a subaccount opens/closes an isolated perpetual position.Adds multiple helper functions to calculate / track the collateral transfers.
Note: This changes the assumption that
UpdateSubaccountsonly changes state in thex/subaccountmodule as it will now change state in thex/bankmodule as well.Test Plan
Unit tests. E2E tests are planned under this ticket.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.