[TRA-125] Update withdrawal gating to be per collateral pool. #1208
Conversation
WalkthroughThe updates focus on enhancing the handling of negative total net collateral (TNC) subaccounts within specific collateral pools across various components of the protocol. This includes adding new parameters to function calls for better identification and management of these subaccounts, introducing methods for querying withdrawal and transfer block status based on collateral pool and perpetual ID, and updating storage keys to reflect the association with specific collateral pools. Additionally, support for isolated perpetuals through new methods and constants has been introduced. 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (10)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (4 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (4 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (6 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (1 hunks)
- protocol/x/subaccounts/types/keys.go (1 hunks)
Additional comments: 12
protocol/x/subaccounts/types/keys.go (1)
- 16-18: The change in the constant name from
NegativeTncSubaccountSeenAtBlockKeytoNegativeTncSubaccountForCollateralPoolSeenAtBlockKeyPrefixis approved. It provides a clearer description of its purpose, aligning with the PR's objectives to implement withdrawal gating on a per-collateral pool basis.protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go (1)
- 28-28: Adding the
types.ModuleAddressparameter to thek.GetNegativeTncSubaccountSeenAtBlockcall aligns with the PR's objectives to implement withdrawal gating on a per-collateral pool basis. Ensure all calls to this function throughout the codebase have been updated to include the new parameter.protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (3)
- 6-6: The addition of the
cosmossdk.io/store/prefiximport is necessary for the implementation of the new gating logic using prefix stores. This aligns with the PR's objectives and is correctly used.- 17-17: The inclusion of
collateralPoolAddr sdk.AccAddressas a parameter in the functionsGetNegativeTncSubaccountSeenAtBlockandSetNegativeTncSubaccountSeenAtBlockis approved. It enables the system to manage negative TNC subaccounts on a per-collateral pool basis, aligning with the PR's objectives.Also applies to: 47-47
- 40-56: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [53-66]
The panic handling in
SetNegativeTncSubaccountSeenAtBlockfor preventing incorrect data updates is approved, given the importance of data integrity in blockchain state management. However, it's recommended to document this behavior clearly to ensure that calling code is aware of the conditions under which the function may panic.+ // SetNegativeTncSubaccountSeenAtBlock sets a block number in state where a negative TNC subaccount + // was seen for a specific collateral pool. This function will overwrite previous values at this key. + // It panics if the old block height is greater than the new block height to ensure data integrity.protocol/x/clob/types/expected_keepers.go (1)
- 64-64: Adding the
collateralPoolAddr sdk.AccAddressparameter to theSetNegativeTncSubaccountSeenAtBlockmethod in theSubaccountsKeeperinterface aligns with the PR's objectives to implement withdrawal gating on a per-collateral pool basis. Ensure all implementations of this interface throughout the codebase have been updated to include the new parameter.protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (2)
- 24-24: The addition of the
types.ModuleAddressparameter in the test functions forGetNegativeTncSubaccountSeenAtBlockandSetNegativeTncSubaccountSeenAtBlockis approved. It ensures the updated logic, which operates on a per-collateral pool basis, is correctly tested.Also applies to: 37-38, 53-54, 62-63, 71-72, 80-81, 99-100, 108-109, 149-153
- 48-48: The update in the storage key from
NegativeTncSubaccountSeenAtBlockKeytoNegativeTncSubaccountForCollateralPoolSeenAtBlockKeyPrefixin the test expectations is approved. It aligns with the modifications in the main codebase and ensures the tests accurately reflect the new logic for managing negative TNC subaccounts on a per-collateral pool basis.Also applies to: 91-94, 119-120
protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (1)
- 51-51: The updates to include
types.ModuleAddressas an argument in thesk.SetNegativeTncSubaccountSeenAtBlockcalls in theTestQueryWithdrawalAndTransfersBlockedInfofunction are approved. It ensures the updated logic, which operates on a per-collateral pool basis, is correctly tested.Also applies to: 99-99, 134-134, 169-169
protocol/x/clob/keeper/process_operations.go (1)
- 697-697: The addition of
satypes.ModuleAddressas a parameter toSetNegativeTncSubaccountSeenAtBlockis crucial for implementing per-collateral pool gating logic. However, it's essential to verify thatsatypes.ModuleAddresscorrectly and dynamically represents the intended collateral pool's address for each operation. Ifsatypes.ModuleAddressis static or not correctly associated with the operation's context, consider obtaining the correct collateral pool address dynamically from the operation's context or other relevant data structures.protocol/x/subaccounts/keeper/subaccount.go (1)
- 559-559: The addition of
types.ModuleAddressas an argument toGetNegativeTncSubaccountSeenAtBlockwithininternalCanUpdateSubaccountsaligns with the PR's objective to implement withdrawal gating on a per-collateral pool basis. This change allows the function to consider the specific collateral pool when determining whether withdrawals or transfers should be blocked due to a negative TNC subaccount being seen. Ensure that thetypes.ModuleAddresspassed here accurately represents the collateral pool intended for this gating logic. Additionally, verify that all calls to this function throughout the codebase have been updated to include this new argument where necessary.protocol/x/clob/keeper/process_operations_test.go (1)
- 2397-2403: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-2400]
The test file
process_operations_test.goprovides a comprehensive suite of test cases for theProcessProposerOperationsfunction in theclobpackage. The tests cover a wide range of scenarios, including successful operations, error handling, and edge cases. The use of a detailed test case structure (processProposerOperationsTestCase) and mock setups ensures thorough validation of the process operations functionality.However, there are a few areas that could benefit from additional attention or improvements:
Mock Expectations Verification: Ensure that all mock expectations set up for the
mockIndexerEventManagerandmockBankKeeperare verified at the end of each test case. This helps ensure that all expected interactions with these mocks occur as intended.Error Messages and Panics: For test cases that expect errors or panics, consider adding more detailed assertions to verify the specific error messages or panic values. This can help ensure that the function fails for the expected reasons.
Code Duplication: There appears to be some duplication in the setup code for the test cases, particularly in the setup of mock expectations and the creation of test entities (e.g., subaccounts, perpetuals). Consider refactoring this setup code into helper functions to reduce duplication and improve maintainability.
Documentation: While the test cases are well-structured and cover a wide range of scenarios, adding more detailed comments or documentation to the test cases and helper functions can improve readability and make it easier for future contributors to understand the test logic.
Performance Considerations: Given the extensive nature of the test suite, consider evaluating the performance of the test execution. If the tests take a significant amount of time to run, it may be worth exploring ways to optimize the test setup or execution.
Overall, the test suite is well-designed and provides valuable coverage for the
ProcessProposerOperationsfunctionality. Addressing the above points can further enhance the quality and maintainability of the tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
Additional comments: 1
protocol/x/clob/e2e/withdrawal_gating_test.go (1)
- 255-255: The addition of
satypes.ModuleAddressas an argument toGetNegativeTncSubaccountSeenAtBlockaligns with the PR's objective to implement withdrawal gating on a per-collateral pool basis. However, it's crucial to ensure thatsatypes.ModuleAddressis correctly defined and used throughout the test to accurately reflect the intended collateral pool. Ifsatypes.ModuleAddressis intended to represent a specific collateral pool, its initialization and relevance to the test scenario should be clearly documented to avoid confusion and ensure the test's validity.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/subaccounts/types/query.pb.gois excluded by:!**/*.pb.goprotocol/x/subaccounts/types/query.pb.gw.gois excluded by:!**/*.pb.gw.go
Files selected for processing (12)
- proto/dydxprotocol/subaccounts/query.proto (1 hunks)
- protocol/testutil/constants/addresses.go (2 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (2 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (11 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (36 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (8)
- protocol/x/clob/e2e/withdrawal_gating_test.go
- protocol/x/clob/keeper/process_operations.go
- protocol/x/clob/keeper/process_operations_test.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
- protocol/x/subaccounts/keeper/subaccount.go
Additional comments: 5
protocol/testutil/constants/addresses.go (2)
- 5-6: The addition of
authtypesandsatypesimports is appropriate for the new functionality introduced in this file.- 23-25: The introduction of
IsoCollateralPoolAddressandIso2CollateralPoolAddressis well-implemented. Consider adding comments to explain the significance of ":3" and ":4" in the module names for future maintainability.proto/dydxprotocol/subaccounts/query.proto (1)
- 59-63: The addition of the
collateral_pool_addressfield toQueryGetWithdrawalAndTransfersBlockedInfoRequestis correctly implemented. It would be beneficial to add documentation comments explaining the purpose of this field and how it is used in the context of querying withdrawal and transfer block status.protocol/x/clob/types/expected_keepers.go (2)
- 64-64: The addition of the
collateralPoolAddrparameter toSetNegativeTncSubaccountSeenAtBlockis well-implemented. Adding documentation comments for this parameter would enhance clarity and maintainability.- 73-76: The introduction of
GetCollateralPoolFromPerpetualIdin theSubaccountsKeeperinterface is correctly implemented. It would be beneficial to add documentation comments explaining the method's purpose and its parameters for future maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.lcd.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.rpc.Query.ts (3 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.ts (4 hunks)
- proto/dydxprotocol/subaccounts/query.proto (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- proto/dydxprotocol/subaccounts/query.proto
Additional comments: 3
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.lcd.ts (1)
- 45-55: The addition of
collateralPoolAddresshandling ingetWithdrawalAndTransfersBlockedInfoaligns with the PR objectives and is implemented correctly. Ensure that the rest of the system integrates this new parameter appropriately.indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.rpc.Query.ts (1)
- 18-18: Making the
requestparameter mandatory ingetWithdrawalAndTransfersBlockedInfois a significant change that aligns with the PR's objectives. Ensure that all existing calls to this method have been updated to comply with the new signature.Also applies to: 44-44, 63-63
Verification successful
The verification process confirms that all calls to
getWithdrawalAndTransfersBlockedInfohave been correctly updated to include the mandatoryrequestparameter, aligning with the changes made in the PR. This ensures that the method's new signature is being used consistently across the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that all calls to getWithdrawalAndTransfersBlockedInfo have been updated. rg --type ts 'getWithdrawalAndTransfersBlockedInfo' --files-with-matches | xargs rg -C3 'request:'Length of output: 6434
indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.ts (1)
- 55-66: The addition of
collateralPoolAddresstoQueryGetWithdrawalAndTransfersBlockedInfoRequestand its handling in related functions are correctly implemented. Ensure that this new field is correctly utilized across the system where relevant.Also applies to: 289-327
Verification successful
The addition of
collateralPoolAddresstoQueryGetWithdrawalAndTransfersBlockedInfoRequestand its SDK type is correctly implemented and utilized across the system. The verification confirms the correct integration and usage of the new field in relevant parts of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify correct usage of collateralPoolAddress across the system. rg --type ts 'collateralPoolAddress' --files-with-matches | xargs rg 'QueryGetWithdrawalAndTransfersBlockedInfoRequest'Length of output: 2811
vincentwschau
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/subaccounts/types/query.pb.gois excluded by:!**/*.pb.go
Files selected for processing (3)
- protocol/testutil/constants/clob_pair.go (1 hunks)
- protocol/testutil/constants/subaccounts.go (2 hunks)
- protocol/x/clob/keeper/process_operations_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/clob/keeper/process_operations_test.go
Additional comments: 5
protocol/testutil/constants/clob_pair.go (3)
- 130-140: LGTM! The addition of
ClobPair_Isois consistent with the existing pattern ofClobPairconstants.- 142-152: LGTM! The
ClobPair_Iso_Final_Settlementconstant correctly represents aClobPairin the final settlement state.- 154-164: LGTM! The addition of
ClobPair_Iso2is consistent and introduces necessary differentiation with uniqueIdandPerpetualId.protocol/testutil/constants/subaccounts.go (2)
- 239-252: LGTM! The addition of
Carl_Num0_1ISO_Short_49USDis consistent with the existing pattern ofSubaccountconstants, with specific asset and perpetual positions well-defined.- 482-494: LGTM! The addition of
Dave_Num0_1ISO2_Short_499USDis consistent and introduces necessary differentiation with a unique perpetual position.
| collateralPoolAddress, err := k.subaccountsKeeper.GetCollateralPoolFromPerpetualId(ctx, perpetualId) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
nit: can maybe just move this to SetNegativeTncSubaccountSeenAtBlock
slightly cleaner if the concept of collateral pool is abstracted away from clob
There was a problem hiding this comment.
up to you though. no strong preference
There was a problem hiding this comment.
+1 to jays comment, related to my comment here, I think i feel a little more strongly than jay haha
| store := prefix.NewStore( | ||
| ctx.KVStore(k.storeKey), | ||
| []byte(types.NegativeTncSubaccountForCollateralPoolSeenAtBlockKeyPrefix), | ||
| ) |
There was a problem hiding this comment.
do we need a state migration or no?
probably does need one I think
There was a problem hiding this comment.
Yep, have a state migration tracked in a separate ticket in the PR description.
| @@ -553,10 +560,17 @@ func (k Keeper) internalCanUpdateSubaccounts( | |||
|
|
|||
| // Block all withdrawals and transfers if either of the following is true within the last | |||
| // `WITHDRAWAL_AND_TRANSFERS_BLOCKED_AFTER_NEGATIVE_TNC_SUBACCOUNT_SEEN_BLOCKS`: | |||
| // - There was a negative TNC subaccount seen. | |||
| // - There was a negative TNC subaccount seen for any of the collateral pools of subaccounts being updated | |||
There was a problem hiding this comment.
for my understanding - we are blocking transfers both from and to the gated collateral pool, right? users also can't transfer to the pool
There was a problem hiding this comment.
Yes, that's correct. Users can deposit into the gated collateral pool though still.
| @@ -127,4 +127,40 @@ var ( | |||
| QuantumConversionExponent: -8, | |||
| Status: clobtypes.ClobPair_STATUS_PAUSED, | |||
| } | |||
| ClobPair_Iso = clobtypes.ClobPair{ | |||
There was a problem hiding this comment.
Nit: ClobPairId_3_Iso, same with below. ClobPair_Iso2 has Id 4 so it gets a little confusing during tests, maybe ClobPairId_4_Iso
| // This function will panic if the old block height is greater than the new block height. | ||
| func (k Keeper) SetNegativeTncSubaccountSeenAtBlock( | ||
| ctx sdk.Context, | ||
| collateralPoolAddr sdk.AccAddress, |
There was a problem hiding this comment.
So collateral pools are tied to sdk.AccAddress, i.e Subaccounts module? shouldn't it be tied to an isolated market?
There was a problem hiding this comment.
Ok, talked to jay, seems like if we use the subaccounts module string that's the default cross margined iso pool.
Can we add this as a comment to make it more clear? also function above.
There was a problem hiding this comment.
Actually, i see here we are making an account address based on "subaccount:<perpId>". Is there a reason we are using an sdk.Addresssdk.AccAddress as the type of the key? Generating an acc address will hash the information so it's unreadable. I think collat pools are based off of perp ids purely. Would it make more sense to take out the concept of an account address from perp ids and use purely a <prefix>:<perp id> for this?
There was a problem hiding this comment.
seems like the only possible params here are <perp id> or <default cross margin perp id> so not sure if we should shoehorn this in an AccAddress, maybe there's a reason we are doing this that I don't see though
There was a problem hiding this comment.
Discussed in slack, the actual place collateral is exiting / entering are sdk.AccAddress which can be derived from a perpetual Id. However, these accounts are associated with the x/subaccounts module so I don't think the addresses should be disassociated from the x/subaccounts module by removing the prefix.
| negativeTncSubaccountSeenAtBlock, exists := tApp.App.SubaccountsKeeper.GetNegativeTncSubaccountSeenAtBlock(ctx) | ||
| negativeTncSubaccountSeenAtBlock, exists := tApp.App.SubaccountsKeeper.GetNegativeTncSubaccountSeenAtBlock( | ||
| ctx, | ||
| satypes.ModuleAddress, |
There was a problem hiding this comment.
nit: can we use a hardcoded constant DEFAULT_CROSS_MARGIN_KEY instead of the module address? i don't think we would change the subaccount module name but it could in the future
There was a problem hiding this comment.
What would the purpose of this be? To ensure that this test continues to pass even if the subaccount module changes?
There was a problem hiding this comment.
just in case the subaccount module name changes, no need for a state migration. up to you
There was a problem hiding this comment.
Changed to a set of suffixes including a hard-coded constant for cross-perpetuals.
| settledUpdates []settledUpdate, | ||
| ) ([]sdk.AccAddress, error) { | ||
| collateralPoolAddressMap := make(map[string]bool) | ||
| collateralPoolAddresses := make([]sdk.AccAddress, 0) |
There was a problem hiding this comment.
nit:
// GetSortedKeys returns the keys of the map in sorted order.
func GetSortedKeys[R interface {
exists or you can make one that doesn't sort
There was a problem hiding this comment.
The keys don't need to be sorted though, are you suggesting that this should be a general helper function rather than a bespoke implementation in here?
Since I'm just de-duplicating here, I guess you're suggesting I add a general helper function to de-duplicate slices?
There was a problem hiding this comment.
yeah general helper function to extract keys of map without sorting, nit though
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
- protocol/x/clob/types/expected_keepers.go (2 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (12 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (4 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (35 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (5)
- protocol/x/clob/keeper/process_operations.go
- protocol/x/clob/types/expected_keepers.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go
Additional comments: 7
protocol/x/subaccounts/keeper/subaccount_test.go (7)
- 2380-2380: The test case "deposits are not blocked if negative TNC subaccount was seen at current block" seems to be missing a description or comment explaining the specific scenario being tested. Adding a brief explanation can improve readability and maintainability.
- 2392-2393: The map
perpetualPositionsis declared but not used in any of the test cases. If it's intended for future use, consider adding a comment to clarify its purpose. Otherwise, it might be cleaner to remove it to avoid confusion.- 2400-2401: Similar to the previous comment,
expectedPerpetualPositionsis declared but not utilized in the test cases provided. Clarification or removal would enhance the code's readability and maintainability.- 2414-2414: The
currentBlockandnegativeTncSubaccountSeenAtBlockfields are crucial for testing the logic related to withdrawal gating based on the negative TNC subaccount's last seen block. However, it would be beneficial to add comments explaining their significance in the context of the tests, especially for new contributors or for future reference.- 3670-3677: Setting the negative TNC subaccount seen at block in state is a critical operation for these tests. It's well-implemented here, but adding inline comments explaining the rationale behind these operations could aid understanding, especially since this setup is key to the logic being tested.
- 3720-3732: The loop iterating over
expectedPerpetualPositionsto assert the equality of expected and actual perpetual positions is clear and logically sound. However, consider adding a brief comment explaining the purpose of this verification step for future maintainers or contributors who might be less familiar with the test's context.- 3727-3732: Similarly, the loop iterating over
expectedAssetPositionsto assert the equality of expected and actual asset positions is well-structured. A brief comment here as well would enhance clarity and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/testutil/constants/clob_pair.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/testutil/constants/clob_pair.go
- protocol/x/clob/keeper/process_operations_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/subaccounts/types/query.pb.gois excluded by:!**/*.pb.go
Files selected for processing (16)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.lcd.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.ts (4 hunks)
- proto/dydxprotocol/subaccounts/query.proto (1 hunks)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
- protocol/x/clob/keeper/process_operations.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (6 hunks)
- protocol/x/clob/types/expected_keepers.go (1 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go (1 hunks)
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go (13 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (35 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/types/keys.go (1 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (11)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.lcd.ts
- indexer/packages/v4-protos/src/codegen/dydxprotocol/subaccounts/query.ts
- protocol/x/clob/e2e/withdrawal_gating_test.go
- protocol/x/clob/keeper/process_operations.go
- protocol/x/clob/keeper/process_operations_test.go
- protocol/x/clob/types/expected_keepers.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info.go
- protocol/x/subaccounts/keeper/grpc_query_withdrawal_and_transfers_blocked_info_test.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/types/keys.go
Additional comments: 14
protocol/x/subaccounts/types/expected_keepers.go (1)
- 77-77: Consider adding documentation for the
IsIsolatedPerpetualmethod to explain its purpose, parameters, and return values. This will enhance code readability and maintainability.proto/dydxprotocol/subaccounts/query.proto (1)
- 62-62: The addition of
perpetual_idtoQueryGetWithdrawalAndTransfersBlockedInfoRequestis clear. Consider adding a comment to document its purpose and usage, enhancing clarity for future developers.protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (1)
- 17-220: The modifications to the tests, including the addition of
perpetualIdand the update to the storage key, align well with the PR's objectives. Ensure comprehensive test coverage for the new logic, possibly including edge cases not covered in these tests.protocol/x/perpetuals/keeper/perpetual.go (5)
- 34-36: The implementation of
IsIsolatedPerpetualmethod is concise and leverages the existingGetInsuranceFundNamemethod effectively to determine if a perpetual is isolated. This approach maintains code modularity and reusability.- 31-41: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [38-1084]
CRUD operations and validation methods in the file are implemented consistently and effectively, utilizing Cosmos SDK stores and custom error handling. These implementations ensure data integrity and provide clear feedback on validation failures.
- 31-41: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1085-1382]
Methods related to premium samples and votes are well-structured and incorporate domain-specific logic effectively. They demonstrate a good understanding of the module's requirements and make appropriate use of the module's configuration parameters.
- 31-41: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1383-1552]
Liquidity tier functions are implemented with attention to detail, ensuring that liquidity tiers are managed effectively within the protocol. These functions play a crucial role in applying margin requirements and other parameters accurately.
- 31-41: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1553-2084]
The file demonstrates a consistent overall structure and includes various utility functions that enhance the module's functionality. It follows best practices for error handling, data validation, and usage of the Cosmos SDK, contributing to the module's robustness and maintainability.
protocol/x/subaccounts/keeper/subaccount_test.go (6)
- 2392-2393: The comment for the
perpetualPositionsmap variable is missing, which could lead to confusion about its purpose and structure. Adding a brief comment explaining its role would improve code readability.- 2400-2401: Similar to the previous comment, the
expectedPerpetualPositionsmap variable lacks a descriptive comment. Providing a short explanation of its expected content and usage would enhance code maintainability.- 2414-2414: The
currentBlockvariable is well-named, indicating its purpose clearly. However, adding a brief comment to explain its role in the context of the tests, especially how it relates to the handling of negative TNC subaccount seen blocks, would be beneficial.- 2449-2452: The usage of
constants.BtcUsd_NoMarginRequirement.Params.Iddirectly in thenegativeTncSubaccountSeenAtBlockmap initialization is clear in its intent. However, a brief comment explaining the significance of setting this specific block number in relation to the test case scenario would aid understanding.- 3670-3678: The loop for setting the negative TNC subaccount seen at block in state is crucial for setting up the test environment correctly. It's implemented correctly, but adding a comment explaining why this setup is necessary for certain test cases (e.g., handling withdrawals and transfers when a negative TNC subaccount has been seen) would be helpful for future maintainers.
- 3721-3733: The final assertion checks in the test cases are critical for verifying the expected outcomes. The logic appears correct, comparing the expected perpetual positions and asset positions with the actual ones retrieved from the subaccount. However, adding comments to explain the significance of these checks in the context of each test case, especially for complex scenarios involving multiple updates and margin requirements, would enhance readability and maintainability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/testutil/constants/subaccounts.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (35 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (2)
- protocol/testutil/constants/subaccounts.go
- protocol/x/subaccounts/keeper/subaccount.go
Additional comments: 11
protocol/x/subaccounts/keeper/subaccount_test.go (11)
- 2612-2612: The test
TestGetCollateralPoolis well-structured and covers various scenarios effectively.- 2612-2612: The test
TestSubaccountGetcorrectly verifies the retrieval functionality for subaccounts.- 2612-2612: The test
TestSubaccountSet_Emptyeffectively verifies the handling of empty subaccounts.- 2612-2612: The test
TestSubaccountGetNonExistentcorrectly verifies the behavior for non-existent subaccounts.- 2612-2612: The test
TestGetAllSubaccounteffectively verifies the functionality to retrieve all subaccounts.- 2612-2612: The test
TestForEachSubaccountcorrectly verifies the iteration functionality over all subaccounts.- 2612-2612: The test
TestUpdateSubaccountsthoroughly covers various scenarios for updating subaccounts. However, consider refactoring to improve readability and maintainability due to its complexity.- 2612-2612: The test
TestUpdateSubaccounts_WithdrawalsBlockedeffectively verifies the behavior when withdrawals are blocked due to negative TNC subaccounts.- 2612-2612: The test
TestCanUpdateSubaccountseffectively verifies the functionality to determine if subaccounts can be updated based on various scenarios.- 2612-2612: The test
TestGetNetCollateralAndMarginRequirementseffectively verifies the functionality to calculate net collateral and margin requirements for subaccounts.- 2612-2612: The test
TestIsValidStateTransitionForUndercollateralizedSubaccount_ZeroMarginRequirementseffectively verifies the validity of state transitions for undercollateralized subaccounts with zero margin requirements.
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/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.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/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
| return nil | ||
| } | ||
|
|
||
| func (k Keeper) getNegativeTncSubaccountStoreSuffx( |
| // slice of `settledUpdate`s passed in. | ||
| // The slice will be de-duplicated and will contain unique collateral pool addresses. | ||
| func (k Keeper) getCollateralPoolAddresses( | ||
| // getNegativeTncSubaccountStoreSuffices gets a slice of negative tnc subaccount store suffices for |
There was a problem hiding this comment.
might needa s/suffices/suffixes the whole file
| @@ -16,4 +16,7 @@ const ( | |||
| // NegativeTncSubaccountForCollateralPoolSeenAtBlockKeyPrefix is the prefix for the store key that | |||
| // stores the last block a negative TNC subaccount was seen in state for a specific collateral pool. | |||
| NegativeTncSubaccountForCollateralPoolSeenAtBlockKeyPrefix = "NegSA:" | |||
| // Suffix for the store key to the last block a negative TNC subaccount was seen in state for the | |||
| // cross collateral pool. | |||
| CrossCollateralSuffix = "cross" | |||
There was a problem hiding this comment.
i think the other suffixes have a colon, should we add that here?
There was a problem hiding this comment.
nvm, i see it is the suffix.
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/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
Additional comments: 7
protocol/x/subaccounts/keeper/negative_tnc_subaccount_test.go (7)
- 17-26: The test setup includes a list of
testPerpetualIdsandtestsuffixesused across multiple test cases. It's good practice to ensure these test inputs are representative of real-world scenarios and edge cases. Consider adding more diverse test cases to cover a broader range of inputs, especially if there are specific edge cases related to the handling of perpetual IDs and collateral pool suffixes.- 35-45: The test case "Block height defaults to zero if not set and doesn't exist" correctly asserts that the block height is zero and does not exist if it hasn't been set. This is a good practice for ensuring the default behavior of the system under test is as expected. However, it's also important to ensure that the test covers scenarios where the block height might be set to a non-zero value by other operations before this test runs, to ensure isolation between tests.
- 52-66: The test case "Block height can be updated" demonstrates updating the block height and verifying the update. This is crucial for ensuring the setter function works as expected. However, consider adding assertions to check that the update does not inadvertently affect other perpetual IDs or collateral pools, ensuring isolation of updates.
- 77-130: The test case "Block height can be updated more than once" is comprehensive in testing multiple updates to the block height. It's good to see multiple updates being tested. To further improve this test, consider adding checks after each update to ensure that intermediate states are as expected, not just the final state. This can help catch issues where intermediate updates may have unintended side effects.
- 143-170: The test case "Block height can be updated to the same block height" checks for idempotency, which is a good practice. However, it would be beneficial to also test the behavior when attempting to update the block height to a lower value than currently set, as this could reveal potential issues with data integrity or error handling in the system.
- 183-210: The test case "Block height can be updated to different block heights for each collateral pool address" is essential for ensuring that updates are isolated to specific collateral pools. This test is well-implemented, but consider adding a scenario where one collateral pool's update fails (e.g., due to an invalid input) to ensure that such failures do not affect other collateral pools' data.
- 250-259: The test
TestGetSetNegativeTncSubaccountSeenAtBlock_PanicsOnDecreasingBlockcorrectly asserts that the system panics when attempting to set a block height to a lower value than the current one. This is an important test for ensuring the system's data integrity. However, ensure that the panic message is precise and informative for easier debugging and maintenance.
…r-collateral-pool
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/x/clob/e2e/withdrawal_gating_test.go (1 hunks)
- protocol/x/clob/keeper/process_operations_test.go (6 hunks)
- protocol/x/perpetuals/keeper/perpetual.go (1 hunks)
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go (3 hunks)
- protocol/x/subaccounts/keeper/subaccount.go (2 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (35 hunks)
- protocol/x/subaccounts/types/expected_keepers.go (1 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review as they are similar to previous changes (6)
- protocol/x/clob/e2e/withdrawal_gating_test.go
- protocol/x/clob/keeper/process_operations_test.go
- protocol/x/perpetuals/keeper/perpetual.go
- protocol/x/subaccounts/keeper/negative_tnc_subaccount.go
- protocol/x/subaccounts/keeper/subaccount.go
- protocol/x/subaccounts/types/expected_keepers.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/clob/keeper/process_operations_test.go (6 hunks)
- protocol/x/subaccounts/keeper/oimf_test.go (1 hunks)
- protocol/x/subaccounts/keeper/subaccount_test.go (35 hunks)
Files not summarized due to errors (1)
- protocol/x/subaccounts/keeper/subaccount_test.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (1)
- protocol/x/subaccounts/keeper/oimf_test.go
Additional comments: 2
protocol/x/clob/keeper/process_operations_test.go (2)
- 70-70: The
expectedNegativeTncSubaccountSeenfield has been modified from a boolean to a map withuint32keys. This change allows tracking the negative TNC subaccount seen status for specificuint32values, enhancing the flexibility and granularity of the test cases.- 961-963: The test case setup now includes assertions for verifying the negative TNC subaccount seen block number. This addition aligns with the updated logic to track this information on a per-collateral pool basis, ensuring that the tests accurately reflect the new functionality.
| @@ -2609,7 +2609,7 @@ func TestUpdateSubaccounts_WithdrawalsBlocked(t *testing.T) { | |||
| Number: 0, | |||
| } | |||
| secondSubaccountId := types.SubaccountId{ | |||
| Owner: "0", | |||
| Owner: "1", | |||
There was a problem hiding this comment.
The variable secondSubaccountId is declared but not used in any of the test cases within this file. Consider removing it if it's not needed to avoid confusion and maintain code cleanliness.
vincentwschau
deleted the
vincentc/tra-125-update-withdrawal-gating-per-collateral-pool
branch
Changelist
Update withdrawal gating logic to be done per collateral pool for negative TNC subaccounts. No changes to logic to block withdrawals after a chain halt.
Test Plan
Unit tests updated. E2E tests update/change tracked in TRA-165. State migration tracked in TRA-167.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.