[OTE-236] Evaluate subaccounts update end state with OIMF #1172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates focus on enhancing the handling and testing of perpetual contracts within a trading protocol, notably by adding new functionalities and improving existing ones. Key changes include the introduction of a method to modify open interest, updates to test utilities for setting up default perpetual order information, and adjustments in handling open interest deltas. Additionally, error handling and testing scenarios have been expanded, alongside code simplifications for better readability and maintenance. 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 (
|
teddyding
force-pushed
the
td/oimf-can-update
branch
2 times, most recently
from
721cd18
to
6b03c0a
Compare
teddyding
changed the title
teddyding
changed the title
teddyding
commented
Mar 19, 2024
| @@ -609,6 +609,17 @@ func (k Keeper) internalCanUpdateSubaccounts( | |||
| } | |||
| } | |||
|
|
|||
| var perpOpenInterestDelta *perptypes.OpenInterestDelta | |||
| // If update type is `Match`, calculate the open interest delta for the updated perpetual. | |||
| // Note: this assumes that `Match` can only happen for perpetuals. | |||
There was a problem hiding this comment.
AFAIK this assumption will hold true for a very long time (we don't have plans to support spot), which is why I didn't include a TODO.
teddyding
commented
Mar 19, 2024
| // Temporily apply open interest delta to perpetuals, so IMF is calculated based on open interest after the update. | ||
| // `perpOpenInterestDeltas` is only present for `Match` update type. | ||
| if perpOpenInterestDelta != nil { | ||
| if err := k.perpetualsKeeper.ModifyOpenInterest( |
There was a problem hiding this comment.
This is the easiest way to do this right now. There's a more "correct" way, which is:
- Modify
ProductKeeperinterface to haveGetMarginRequirements()also take in anopenInterestDelta(used here in the generic helpercalculate) - Modify the input
size types.Positionto also return theopenInterestDeltaassociated with this size. This is because margin requirements no longer just dependent total size of position, but also on the total open interest
I didn't do this because of the simplicity of the alternative approach used in this PR. Lmk if there's any preference.
Note: Updating perp.OpenInterest += delta and then perp.OpenInterest -= delta may modify app hash. We don't need to make any assumption here, since for DeliverTx this is called deterministically. For CheckTx, no need to keep each node CheckState the same either.
teddyding
force-pushed
the
td/oimf-can-update
branch
from
6b03c0a
to
30bd1de
Compare
![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 selected for processing (19)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/testutil/constants/perpetuals.go (4 hunks)
- protocol/testutil/perpetuals/perpetuals.go (2 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (8 hunks)
- protocol/x/clob/keeper/liquidations_test.go (8 hunks)
- protocol/x/clob/keeper/orders.go (1 hunks)
- protocol/x/clob/keeper/orders_test.go (3 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (1 hunks)
- protocol/x/perpetuals/types/types.go (2 hunks)
- protocol/x/subaccounts/keeper/isolated_subaccount.go (2 hunks)
- protocol/x/subaccounts/keeper/oimf.go (1 hunks)
- protocol/x/subaccounts/keeper/oimf_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (9 hunks)
- protocol/x/subaccounts/keeper/subaccount_helper.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
- protocol/x/subaccounts/keeper/update.go (1 hunks)
- protocol/x/subaccounts/types/errors.go (2 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/types/update.go (1 hunks)
Files skipped from review as they are similar to previous changes (18)
- protocol/mocks/PerpetualsKeeper.go
- protocol/testutil/constants/perpetuals.go
- protocol/testutil/perpetuals/perpetuals.go
- protocol/x/clob/keeper/deleveraging_test.go
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/keeper/orders.go
- protocol/x/clob/keeper/orders_test.go
- protocol/x/perpetuals/keeper/perpetual.go
- protocol/x/perpetuals/types/types.go
- protocol/x/subaccounts/keeper/isolated_subaccount.go
- protocol/x/subaccounts/keeper/oimf.go
- protocol/x/subaccounts/keeper/oimf_test.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/keeper/subaccount_helper.go
- protocol/x/subaccounts/keeper/update.go
- protocol/x/subaccounts/types/errors.go
- protocol/x/subaccounts/types/expected_keepers.go
- protocol/x/subaccounts/types/update.go
jayy04
reviewed
Mar 20, 2024
Comment on lines
672
to
689
| // Revert the temporary open interest delta to perpetuals. | ||
| if perpOpenInterestDelta != nil { | ||
| if err := k.perpetualsKeeper.ModifyOpenInterest( | ||
| ctx, | ||
| perpOpenInterestDelta.PerpetualId, | ||
| new(big.Int).Neg( | ||
| perpOpenInterestDelta.BaseQuantumsDelta, | ||
| ), | ||
| ); err != nil { | ||
| return false, nil, errorsmod.Wrapf( | ||
| types.ErrCannotRevertPerpOpenInterestForOIMF, | ||
| "perpId = %v, delta = %v, settledUpdates = %+v", | ||
| perpOpenInterestDelta.PerpetualId, | ||
| perpOpenInterestDelta.BaseQuantumsDelta, | ||
| settledUpdates, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
branch the context instead? seems a little bit cleaner
up to you
jayy04
reviewed
Mar 20, 2024
Comment on lines
3250
to
3253
| Id: &types.SubaccountId{ | ||
| Owner: "Bob", | ||
| Number: 0, | ||
| }, |
There was a problem hiding this comment.
mega nit: just use constants.Bob_Num0
jayy04
approved these changes
Mar 20, 2024
| ) | ||
| } | ||
|
|
||
| if settledUpdates[0].PerpetualUpdates[0].PerpetualId != settledUpdates[1].PerpetualUpdates[0].PerpetualId { |
There was a problem hiding this comment.
nit: The 2 perpetual updates should also have the same absolute value as their size right? If a subaccount is chaing by +1 BTC from a match, the other subaccount should be changing -1 BTC.
|
non-blocking nit, assume it may already to scoped to a different ticket. It would be good to update / add some E2E tests for this logic as well. |
vincentwschau
approved these changes
Mar 20, 2024
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/subaccount.go (9 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/keeper/subaccount_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/subaccounts/keeper/oimf.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
- protocol/x/subaccounts/types/errors.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/oimf.go
- protocol/x/subaccounts/types/errors.go
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/oimf.go (1 hunks)
- protocol/x/subaccounts/keeper/oimf_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/oimf.go
- protocol/x/subaccounts/keeper/oimf_test.go
teddyding
force-pushed
the
td/oimf-can-update
branch
from
d332cb5
to
2c9f80a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/x/subaccounts/keeper/oimf.go (1 hunks)
- protocol/x/subaccounts/keeper/oimf_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
- protocol/x/subaccounts/types/errors.go (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- protocol/x/subaccounts/keeper/oimf.go
- protocol/x/subaccounts/keeper/oimf_test.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/types/errors.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/subaccount_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/x/clob/keeper/process_operations_liquidations_test.go (37 hunks)
- protocol/x/clob/keeper/process_operations_long_term_test.go (11 hunks)
- protocol/x/clob/keeper/process_operations_stateful_validation_test.go (16 hunks)
- protocol/x/clob/keeper/process_operations_test.go (41 hunks)
Additional comments: 56
protocol/x/clob/keeper/process_operations_stateful_validation_test.go (2)
- 29-29: The changes from lines 29 to 327 in
TestProcessProposerMatches_LongTerm_StatefulValidation_Failureinvolve converting slices of pointers to structs to slices of structs for theperpetualsfield. This is a positive change for performance and code clarity. Ensure that this change aligns with howperpetualsare used within the test logic.- 396-396: The changes from lines 396 to 623 in
TestProcessProposerMatches_Conditional_Validation_Failurealso involve converting slices of pointers to structs to slices of structs for theperpetualsfield. As with the previous function, this is a beneficial change. Ensure that this change aligns with howperpetualsare used within the test logic.protocol/x/clob/keeper/process_operations_long_term_test.go (5)
- 27-27: The changes from lines 27 to 826 involve updating the type of
perpetualsandsubaccountsslices from pointers to values. This is a significant change that affects how these slices are used throughout the test cases. Ensure that all operations on these slices are correctly adjusted to work with value semantics instead of pointer semantics. This includes checking for any potential unintended copies or modifications that could affect test behavior.- 97-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-826]
The test cases introduced and modified in this file cover a wide range of scenarios related to the handling of long-term orders, including new maker and taker orders, liquidation matches, and considering state fill amounts. It's crucial to ensure that these test cases comprehensively cover the changes introduced in the PR, especially the enhancements to subaccount updates and the integration of the Open Interest Modification Framework (OIMF). Additionally, consider adding or updating end-to-end (E2E) tests as suggested by
vincentwschauto further validate the changes.Would you like assistance in adding or updating the E2E tests to cover the new logic introduced in this PR?
- 97-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-826]
Ensure that error handling is properly implemented for all setup operations and external calls within the test cases. This includes mocking external dependencies and setting up initial states. Proper error handling is essential for maintaining test reliability and ensuring that failures are accurately reported.
- 97-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-826]
Consider refactoring the test setup code to reduce duplication and improve maintainability. Common setup operations, such as mocking the bank keeper and setting the previous block info, are repeated across multiple test cases. Extracting these operations into helper functions could make the test code more concise and easier to maintain.
- 97-104: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [27-826]
The test function
TestProcessProposerMatches_LongTerm_Successdemonstrates good use of a map to define and run multiple test cases, enhancing modularity. This approach allows for easy addition of new test cases and improves the readability of the test suite. Keep this structure in mind for future test implementations to maintain consistency and modularity.protocol/x/clob/keeper/process_operations_liquidations_test.go (9)
- 34-36: The conversion from slices of pointers to slices of structs for
perpetualssimplifies the code and potentially improves performance by avoiding indirect references. This change enhances code readability and maintainability.- 166-172: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [169-183]
The conversion from slices of pointers to slices of structs for
perpetualsandsubaccountsin this test case also simplifies the code. It's a good practice to use struct instances directly when the overhead of copying is not significant, which seems to be the case here.
- 261-262: Similar to previous comments, converting to slices of structs for
perpetualshere is a positive change. It simplifies the handling of these objects within the test cases.- 347-348: Again, the conversion to slices of structs for
perpetualsis consistent with the changes made throughout the file. This consistency in handlingperpetualsandsubaccountsas struct slices is beneficial for code clarity.- 445-446: The consistent application of converting to slices of structs for
perpetualsacross different test cases is noted and appreciated. This change contributes to a more uniform and readable codebase.- 550-551: The conversion to slices of structs for
perpetualshere, as in other test cases, is a good practice. It simplifies the code structure and makes it more straightforward to work with these objects.- 654-655: The conversion from slices of pointers to slices of structs for
perpetualsin this test case is consistent with the rest of the file. This change is beneficial for the reasons previously mentioned.- 767-768: Once again, converting to slices of structs for
perpetualsis a positive change that simplifies the code. This consistency across test cases is good for maintainability.- 866-867: The conversion to slices of structs for
perpetualsin this test case is in line with the changes throughout the file. This simplification is beneficial for code readability.protocol/x/clob/keeper/process_operations_test.go (40)
- 23-23: The import
perptest "github.com/dydxprotocol/v4-chain/protocol/testutil/perpetuals"is added, which is necessary for the test setup, particularly for setting up default perpetual open interests.- 45-45: The change from
[]*perptypes.Perpetualto[]perptypes.Perpetualin theperpetualsslice type within theprocessProposerOperationsTestCasestruct simplifies the handling of perpetuals by avoiding the need for pointer dereferencing. This change aligns with Go best practices for slices of structs where the structs are not large or where the slice does not undergo frequent modifications that would benefit from pointer semantics.- 79-79: The test case "Succeeds no operations" correctly initializes an empty slice of
perptypes.Perpetualto test the scenario where no operations are processed. This is a valid test case to ensure the function behaves correctly when no input is provided.- 91-92: The test case "Succeeds no operations with previous stateful orders" demonstrates the function's behavior when stateful orders exist but no new operations are processed. The inclusion of a perpetual in the test setup is necessary for the context of the test, ensuring that the system's state is accurately represented.
- 110-111: The test case "Succeeds with singular match of a short term maker and short term taker" is well-constructed to test the matching logic between two short-term orders. This case is essential for verifying the core functionality of order matching within the system.
- 234-235: The test case "Succeeds with maker rebate" introduces a scenario where a maker rebate is applied, testing the system's ability to handle fee adjustments based on order roles. This is an important aspect of the trading system that needs thorough testing.
- 358-359: The test case "Succeeds with singular match of a preexisting maker and short term taker" tests the interaction between a preexisting long-term order and a newly placed short-term order. This scenario is crucial for ensuring that the system can correctly handle matches involving different types of orders.
- 469-470: The test case "Succeeds with singular match of a preexisting maker and newly placed long term taker" is designed to test the matching between two long-term orders, one of which is preexisting. This scenario ensures that the system's logic for handling long-term orders is functioning as expected.
- 561-562: The test case "preexisting stateful maker order partially matches with 2 short term taker orders" is a complex scenario that tests the system's ability to handle partial matches involving multiple orders. This test is essential for verifying the robustness of the matching engine.
- 735-736: The test case "Succeeds with liquidation order" introduces a liquidation scenario, testing the system's ability to handle liquidations correctly. This is a critical aspect of the trading system that ensures the financial integrity of the market.
- 801-802: The test case "Succeeds with deleveraging with no liquidation order" tests the deleveraging process in the absence of a liquidation match. This scenario is important for ensuring that deleveraging can occur under specific market conditions.
- 851-852: The test case "Succeeds with deleveraging and partially filled liquidation" tests a complex scenario involving both deleveraging and liquidation processes. This test is crucial for ensuring that the system can handle multiple types of operations simultaneously.
- 928-929: The test case "Zero-fill deleveraging succeeds when the account is negative TNC and updates the last negative TNC subaccount seen block number in state" tests the system's ability to handle zero-fill deleveraging for accounts with negative total net collateral (TNC). This scenario is important for ensuring the system's resilience in adverse market conditions.
- 965-966: The test case "Zero-fill deleveraging succeeds when the account is negative TNC and has a position in final settlement market. It updates the last negative TNC subaccount seen block number in state" extends the previous test case to include accounts in final settlement markets. This adds an additional layer of complexity to the test, ensuring the system's comprehensive handling of deleveraging scenarios.
- 1005-1006: The test case "Zero-fill deleveraging succeeds when there's multiple zero-fill deleveraging events for the same subaccount and perpetual ID" tests the system's ability to handle multiple deleveraging events for the same account. This scenario is important for ensuring that the system can correctly process multiple deleveraging operations in a single block.
- 1048-1049: The test case "Zero-fill deleverage succeeds after the same subaccount is partially deleveraged" tests the system's ability to handle a sequence of deleveraging events, where an account is partially deleveraged before a zero-fill deleveraging occurs. This scenario is crucial for testing the system's handling of complex deleveraging sequences.
- 1110-1111: The test case "Succeeds order removal operations with previous stateful orders" tests the system's ability to remove orders from the state. This scenario is important for ensuring that the system can correctly update its state in response to order removal operations.
- 1143-1144: The test case "Fails when attempting to match order with invalid order side" introduces a scenario where an order with an unspecified side is processed. This test is crucial for ensuring that the system correctly handles invalid input and returns the appropriate error.
- 1218-1219: The test case "Fails with deleveraging match for non-liquidatable subaccount" tests the system's response to a deleveraging operation for a subaccount that is not eligible for liquidation. This scenario is important for ensuring that the system correctly identifies and rejects invalid deleveraging operations.
- 1256-1257: The test case "Fails with deleveraging match for subaccount with zero TNC" tests the system's response to a deleveraging operation for a subaccount with zero total net collateral. This scenario is important for ensuring that the system correctly identifies and rejects deleveraging operations for subaccounts that do not meet the criteria for deleveraging.
- 1295-1296: The test case "Conditional: succeeds with singular match of a triggered conditional order" tests the system's ability to process matches involving triggered conditional orders. This scenario is important for ensuring that the system can correctly handle conditional orders once they are triggered.
- 1389-1390: The test case "Conditional: panics with a non-existent conditional order" introduces a scenario where a match operation is attempted with a conditional order that does not exist in the system. This test is crucial for ensuring that the system correctly identifies and handles attempts to match non-existent orders, although the use of panic for control flow in production code is generally discouraged and should be handled through error returns.
Consider replacing panic with error handling to improve the robustness and maintainability of the code.
- 1443-1444: The test case "Conditional: panics with an untriggered conditional order" tests the system's response to an attempt to match an untriggered conditional order. This scenario is important for ensuring that the system correctly identifies and rejects matches involving untriggered conditional orders, but again, the use of panic for control flow should be reconsidered.
Consider replacing panic with error handling to improve the robustness and maintainability of the code.
- 1498-1499: The test case "Fails with clob pair not found" tests the system's response to an operation involving a clob pair that does not exist in the system. This scenario is important for ensuring that the system correctly identifies and rejects operations involving non-existent clob pairs.
- 1516-1517: The test case "Panics with unsupported clob pair status" introduces a scenario where an operation is attempted with a clob pair in an unsupported status. This test is crucial for ensuring that the system correctly identifies and handles unsupported clob pair statuses, but the use of panic for control flow should be reconsidered.
Consider replacing panic with error handling to improve the robustness and maintainability of the code.
- 1541-1542: The test case "Returns error if zero-fill deleveraging operation proposed for non-negative TNC subaccount in final settlement" tests the system's response to a zero-fill deleveraging operation for a non-negative TNC subaccount in a final settlement market. This scenario is important for ensuring that the system correctly identifies and rejects invalid zero-fill deleveraging operations.
- 1577-1578: The test case "Fails with clob match for market in initializing mode" tests the system's response to a match operation for a market in initializing mode. This scenario is important for ensuring that the system correctly identifies and rejects operations for markets in unsupported statuses.
- 1598-1599: The test case "Fails with short term order placement for market in initializing mode" tests the system's response to a short-term order placement for a market in initializing mode. This scenario is important for ensuring that the system correctly identifies and rejects short-term order placements for markets in unsupported statuses.
- 1613-1614: The test case "Fails with order removal for market in initializing mode" tests the system's response to an order removal operation for a market in initializing mode. This scenario is important for ensuring that the system correctly identifies and rejects order removal operations for markets in unsupported statuses.
- 1632-1633: The test case "Fails with order removal reason fully filled" tests the system's response to an order removal operation with the reason "fully filled". This scenario is important for ensuring that the system correctly identifies and rejects invalid reasons for order removal.
- 1651-1652: The test case "Fails with order removal for market in final settlement" tests the system's response to an order removal operation for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects order removal operations for markets in unsupported statuses.
- 1667-1668: The test case "Fails with short-term order placement for market in final settlement" tests the system's response to a short-term order placement for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects short-term order placements for markets in unsupported statuses.
- 1682-1683: The test case "Fails with ClobMatch_MatchOrders for market in final settlement" tests the system's response to a match operation for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects match operations for markets in unsupported statuses.
- 1705-1706: The test case "Fails with ClobMatch_MatchPerpetualLiquidation for market in final settlement" tests the system's response to a perpetual liquidation match operation for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects liquidation operations for markets in unsupported statuses.
- 1734-1735: The test case "Succeeds with ClobMatch_MatchPerpetualDeleveraging, IsFinalSettlement is false for market in final settlement" tests the system's ability to process a deleveraging operation for a market in final settlement with the
IsFinalSettlementflag set to false. This scenario is important for ensuring that the system can correctly handle deleveraging operations in final settlement markets.- 1779-1780: The test case "Succeeds with ClobMatch_MatchPerpetualDeleveraging, IsFinalSettlement is true for market in final settlement" tests the system's ability to process a deleveraging operation for a market in final settlement with the
IsFinalSettlementflag set to true. This scenario is important for ensuring that the system can correctly handle final settlement operations.- 1822-1823: The test case
Fails with ClobMatch_MatchPerpetualDeleveraging for negative TNC subaccount, IsFinalSettlement is true for market not in final settlementtests the system's response to a deleveraging operation with theIsFinalSettlementflag set to true for a market not in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects deleveraging operations with mismatched final settlement flags.- 1859-1860: The test case
Fails with ClobMatch_MatchPerpetualDeleveraging for negative TNC subaccount, IsFinalSettlement is true for market in final settlementtests the system's response to a deleveraging operation with theIsFinalSettlementflag set to true for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects deleveraging operations with mismatched final settlement flags.- 1898-1899: The test case
Fails with ClobMatch_MatchPerpetualDeleveraging for non-negative TNC subaccount, IsFinalSettlement is true for market not in final settlementtests the system's response to a deleveraging operation with theIsFinalSettlementflag set to true for a market not in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects deleveraging operations with mismatched final settlement flags.- 1931-1932: The test case
Fails with ClobMatch_MatchPerpetualDeleveraging for non-negative TNC subaccount, IsFinalSettlement is false for market in final settlementtests the system's response to a deleveraging operation with theIsFinalSettlementflag set to false for a market in final settlement. This scenario is important for ensuring that the system correctly identifies and rejects deleveraging operations with mismatched final settlement flags.
teddyding
force-pushed
the
td/oimf-can-update
branch
from
1e2a394
to
ecd802a
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/testutil/constants/perpetuals.go (4 hunks)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go (18 hunks)
- protocol/x/perpetuals/genesis.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/constants/perpetuals.go
Additional comments: 2
protocol/x/perpetuals/genesis.go (1)
- 38-44: The changes from lines 38 to 44 replace
CreatePerpetualwithSetPerpetualto allow initializing perpetuals with non-zero values at genesis. This is a significant improvement for flexibility. However, ensure thatSetPerpetualincludes comprehensive initialization logic and error handling thatCreatePerpetualmight have provided. This includes validating input data and setting any default values if necessary.protocol/x/clob/e2e/liquidation_deleveraging_test.go (1)
- 68-68: The changes from lines 68 to 68, 118 to 118, 166 to 166, 228 to 228, 290 to 290, 356 to 356, 428 to 428, 501 to 501, 673 to 673, 715 to 715, 774 to 774, 815 to 815, 872 to 872, 904 to 904, 960 to 960, 1025 to 1025, 1059 to 1059, and 1088 to 1088 update the perpetual configuration constants used in various test cases. This change aligns with the PR objectives to enhance subaccount handling and integrate the Open Interest Modification Framework (OIMF) for evaluating subaccount updates. It's crucial to ensure that these new constants (
BtcUsd_20PercentInitial_10PercentMaintenance_OpenInterest1) are correctly defined elsewhere in the codebase and accurately represent the intended testing scenarios. Additionally, verify that all relevant test cases have been updated to use the new constants for consistency.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (22)
- protocol/mocks/PerpetualsKeeper.go (1 hunks)
- protocol/testutil/constants/perpetuals.go (4 hunks)
- protocol/testutil/perpetuals/perpetuals.go (2 hunks)
- protocol/x/clob/e2e/liquidation_deleveraging_test.go (18 hunks)
- protocol/x/clob/keeper/deleveraging_test.go (8 hunks)
- protocol/x/clob/keeper/liquidations_test.go (8 hunks)
- protocol/x/clob/keeper/orders.go (1 hunks)
- protocol/x/clob/keeper/orders_test.go (3 hunks)
- protocol/x/clob/keeper/process_operations_liquidations_test.go (37 hunks)
- protocol/x/clob/keeper/process_operations_long_term_test.go (11 hunks)
- protocol/x/clob/keeper/process_operations_stateful_validation_test.go (16 hunks)
- protocol/x/clob/keeper/process_operations_test.go (41 hunks)
- protocol/x/perpetuals/genesis.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (1 hunks)
- protocol/x/perpetuals/types/types.go (2 hunks)
- protocol/x/subaccounts/keeper/oimf.go (1 hunks)
- protocol/x/subaccounts/keeper/oimf_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (21 hunks)
- protocol/x/subaccounts/types/errors.go (2 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/types/update.go (1 hunks)
Files skipped from review as they are similar to previous changes (22)
- protocol/mocks/PerpetualsKeeper.go
- protocol/testutil/constants/perpetuals.go
- protocol/testutil/perpetuals/perpetuals.go
- protocol/x/clob/e2e/liquidation_deleveraging_test.go
- protocol/x/clob/keeper/deleveraging_test.go
- protocol/x/clob/keeper/liquidations_test.go
- protocol/x/clob/keeper/orders.go
- protocol/x/clob/keeper/orders_test.go
- protocol/x/clob/keeper/process_operations_liquidations_test.go
- protocol/x/clob/keeper/process_operations_long_term_test.go
- protocol/x/clob/keeper/process_operations_stateful_validation_test.go
- protocol/x/clob/keeper/process_operations_test.go
- protocol/x/perpetuals/genesis.go
- protocol/x/perpetuals/keeper/perpetual.go
- protocol/x/perpetuals/types/types.go
- protocol/x/subaccounts/keeper/oimf.go
- protocol/x/subaccounts/keeper/oimf_test.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/keeper/subaccount_test.go
- protocol/x/subaccounts/types/errors.go
- protocol/x/subaccounts/types/expected_keepers.go
- protocol/x/subaccounts/types/update.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/testing/e2e/gov/wind_down_market_test.go (1 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (2 hunks)
Additional comments: 3
protocol/testing/e2e/gov/wind_down_market_test.go (1)
- 149-149: The change from
BtcUsd_20PercentInitial_10PercentMaintenancetoBtcUsd_20PercentInitial_10PercentMaintenance_OpenInterest1aligns with the PR's focus on evaluating subaccount updates using OIMF. Ensure that the new identifier is correctly defined and set up in the test constants or configurations.protocol/x/clob/e2e/withdrawal_gating_test.go (2)
- 76-76: The update to the
perpetualsconstant to include open interest considerations is consistent with the PR's objectives. Ensure thatBtcUsd_20PercentInitial_10PercentMaintenance_OpenInterest1is correctly defined and set up in the test constants or configurations.- 115-115: The update to the
perpetualsconstant to include open interest considerations is consistent with the PR's objectives. Ensure thatBtcUsd_20PercentInitial_10PercentMaintenance_OpenInterest1is correctly defined and set up in the test constants or configurations.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.