[TRA-80] Update subaccount-to-subaccount transfers for collateral pools. #1178
Conversation
…n collateral pools.
WalkthroughThe updates across various files mainly focus on enhancing the subaccount functionalities within a protocol, specifically by introducing and testing a new method for transferring funds between subaccounts. These changes span across updating the code generation tool, adding panic statements for error handling, and refining the logic for fund transfers, including the handling of different collateral pools and asset types. Documentation improvements and the addition of specific market and position constants also form part of these updates. 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: 2
Configuration used: CodeRabbit UI
Files selected for processing (8)
- protocol/mocks/SubaccountsKeeper.go (10 hunks)
- protocol/testutil/constants/perpetuals.go (1 hunks)
- protocol/testutil/constants/positions.go (1 hunks)
- protocol/x/sending/keeper/transfer.go (1 hunks)
- protocol/x/sending/types/expected_keepers.go (1 hunks)
- protocol/x/subaccounts/keeper/transfer.go (1 hunks)
- protocol/x/subaccounts/keeper/transfer_test.go (1 hunks)
- protocol/x/subaccounts/types/types.go (1 hunks)
Additional comments: 8
protocol/x/subaccounts/types/types.go (1)
- 54-60: The addition of
TransferFundsFromSubaccountToSubaccountto theSubaccountsKeeperinterface is clear and aligns with the PR objectives. However, it would be beneficial to add documentation for this method to maintain consistency and clarity within the interface.protocol/x/sending/types/expected_keepers.go (1)
- 56-62: The addition of
TransferFundsFromSubaccountToSubaccountto theSubaccountsKeeperinterface in this file is consistent with the changes across the codebase. To enhance clarity and maintain consistency, adding documentation for this new method would be beneficial.protocol/testutil/constants/positions.go (1)
- 82-97: The addition of new constants for perpetual positions in isolated markets is correctly implemented and aligns with the PR's objectives to enhance collateral pool management. The naming convention and struct usage are consistent and clear.
protocol/x/sending/keeper/transfer.go (1)
- 25-31: The modification to use
TransferFundsFromSubaccountToSubaccountin theProcessTransferfunction simplifies and potentially improves the efficiency of transfers between subaccounts. It would be helpful to add a comment explaining the rationale behind this change for future maintainers.protocol/mocks/SubaccountsKeeper.go (1)
- 195-211: The update to include
TransferFundsFromSubaccountToSubaccountin the mockSubaccountsKeeperand the addition of panic statements for unhandled return values are correctly implemented. These changes support testing of the new functionality and follow common practices for mock testing.protocol/testutil/constants/perpetuals.go (1)
- 382-382: The addition of
Iso2Usd_IsolatedMarketto theTestMarketPerpetualsslice introduces a new isolated market perpetual for testing. This is a straightforward addition and follows the pattern established by other perpetual market constants in the file. It's important for testing infrastructure to keep pace with the features and configurations supported by the application, and this addition appears to do just that.Ensure that any new features or configurations introduced in the main application are also reflected in the test constants and utilities to maintain comprehensive test coverage and facilitate accurate testing scenarios.
protocol/x/subaccounts/keeper/transfer_test.go (2)
- 577-725: The tests for
TestTransferFundsFromSubaccountToSubaccount_Successcover various scenarios of transferring funds between subaccounts, including isolated and non-isolated subaccounts. It's good to see comprehensive test coverage for different types of transfers. However, consider adding a test case for transferring a zero amount to verify that the function handles this edge case correctly, if applicable to the business logic.- 845-1051: The tests for
TestTransferFundsFromSubaccountToSubaccount_Failureeffectively cover failure scenarios, including insufficient funds and unsupported assets. It's crucial to ensure that all error paths are tested to maintain robustness. One suggestion is to verify that the state of the system remains unchanged after a failed transfer attempt. This can be done by checking the balances of both the sender and recipient subaccounts before and after the operation to ensure they are not altered in case of failure.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testing/e2e/funding/funding_e2e_test.go (1 hunks)
Additional comments: 2
protocol/testing/e2e/funding/funding_e2e_test.go (2)
- 486-486: The gas value for transaction processing has been increased to 110,000 units. While this change is likely necessary to accommodate more complex transactions or state changes, it's important to ensure that this increase aligns with the expected transaction complexity and doesn't inadvertently mask inefficiencies in the code.
- 483-489: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-486]
The
TestFundingfunction is comprehensive and covers a wide range of scenarios. To improve readability and maintainability, consider abstracting some of the setup logic into helper functions or adding more comments to describe the purpose and expected outcomes of each test case. This will make it easier for future contributors to understand and modify the test suite.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/testutil/constants/positions.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/testutil/constants/positions.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/transfer_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/subaccounts/keeper/transfer_test.go
| quantums: big.NewInt(500), | ||
| expectedErr: types.ErrFailedToUpdateSubaccounts, | ||
| }, | ||
| "Send from non-isolated subaccount to isolated subaccount, collateral pool does not have enough balance": { |
There was a problem hiding this comment.
for my understanding - collateral pools should never not have enough balance, right? should we panic instead of returning an error?
There was a problem hiding this comment.
Collateral pools could end up with not enough balance, in the same way the subaccounts module account could end up with not enough balance if withdrawal gating doesn't catch a negative TNC subaccount in time.
In terms of panic'ing, I think that could be fine, however it would diverge from the withdrawal flow, which returns an error.
Changelist
Added a new function to
x/subaccountskeeper to transfer funds from subaccount to subaccount and send coins between collateral pools. Updatedx/sendingto use the new function instead ofUpdateSubaccounts.Test Plan
Added new unit tests.
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.