[CORE-619] add e2e tests for subaccount transfers #800
Conversation
WalkthroughThe changes involve the addition of a new test function Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/sending/app_test.go (1 hunks)
Additional comments: 1
protocol/x/sending/app_test.go (1)
- 30-96: The test cases in
TestMsgCreateTransferare well-structured and cover a variety of scenarios. However, there are a few points to consider:
- The test case "Success: transfer from Alice subaccount to non-existent subaccount" assumes that transferring to a non-existent subaccount will create it. This behavior should be explicitly documented in the system's design or API documentation, as it could have security implications.
- The test case "Failure: transfer a non-USDC asset" uses a string "Non-USDC asset transfer not implemented" to check for an error condition. It's important to ensure that this string is consistent with the actual error message returned by the system.
- The test case "Failure: transfer zero amount" and "Failure: transfer from a subaccount to itself" are good checks for input validation. Ensure that these checks are also performed at the API level and not just in the tests to prevent these operations from occurring in a production environment.
Overall, the tests seem to be comprehensive, but it's crucial to ensure that the assumptions made in the tests align with the actual system behavior and that all error conditions are handled consistently across the application.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/sending/app_test.go (2 hunks)
Additional comments: 2
protocol/x/sending/app_test.go (2)
3-11: The import block has been updated to include necessary packages for the test cases. Ensure that all new imports are used within the test file to avoid including unnecessary dependencies.
32-307: The
TestMsgCreateTransferfunction has been added to test various scenarios for transferring assets between subaccounts. The test cases are well-structured and cover a range of expected behaviors. However, there are a few points to consider:
- Ensure that the
senderInitialBalanceis set up correctly in the genesis state for each test case to reflect the initial conditions accurately.- Verify that the
checkTxResponseContainsstring is correctly asserted in the test cases that expect a specific error message.- Confirm that the
CheckTxandDeliverTxmethods are being called with the correct parameters and that their responses are being handled as expected.- Check that the balance assertions after the transfer (
senderQuantumsAfterTransferandrecipientQuantumsAfterTransfer) are correctly calculated and that the test fails if the balances do not match the expected values.- Ensure that the indexer events are being checked correctly and that the expected events match the actual events emitted during the test.
Overall, the test function appears to be comprehensive and should provide good coverage for the transfer functionality. It is important to run the tests and verify that they pass and that they are correctly testing the intended behavior.
| amount: 7_654_321, | ||
| }, | ||
| // Transfer to a non-existent subaccount will create that subaccount and succeed. | ||
| "Success: transfer from Alice subaccount to non-existent subaccount": { |
There was a problem hiding this comment.
Bob's account already exists on the chain, though, yes? Asking b/c I'm not sure - does it make sense to run a test case with an unused account id to create a subaccount? Are there any owner checks?
There was a problem hiding this comment.
oh actually I overrode genesis state with just 1 subaccount (sender subaccount) so all test cases actually send to a subaccount that doesn't exist yet. Let me update the PR!
There was a problem hiding this comment.
ICICIC. Thanks for explaining!
| } | ||
| require.Conditionf(t, checkTxResp.IsOK, "Expected CheckTx to succeed. Response: %+v", checkTxResp) | ||
|
|
||
| // Check that no indexer events are emitted so far. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/sending/app_test.go (2 hunks)
Additional comments: 6
protocol/x/sending/app_test.go (6)
3-9: The addition of import statements for "bytes" and "github.com/cometbft/cometbft/types" is consistent with the summary and necessary for the new tests.
32-329: The new test function
TestMsgCreateTransferis well-structured and covers a comprehensive set of scenarios for asset transfers between subaccounts, including success and failure cases.63-120: The test cases within
TestMsgCreateTransfercorrectly implement the scenarios described in the summary, such as transfers between subaccounts, to non-existent subaccounts, non-USDC assets, zero amounts, and transfers from a subaccount to itself.123-328: The test setup and assertions in
TestMsgCreateTransferare thorough, checking not only for the correct transaction execution but also for the proper state updates and event generation.63-120: The use of constants for asset IDs and subaccount IDs in the test cases enhances maintainability and readability.
214-326: The test function
TestMsgCreateTransfercorrectly checks for the presence of indexer events, ensuring that the system's indexing functionality is working as expected.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/sending/app_test.go (2 hunks)
Additional comments: 4
protocol/x/sending/app_test.go (4)
3-9: The addition of the "bytes" and "github.com/cometbft/cometbft/types" packages is justified as they are used within the new test function
TestMsgCreateTransfer.32-328: The new test function
TestMsgCreateTransferis well-structured, with clear setup and expectations for each test case, covering a variety of scenarios for transfer operations.123-328: The test function includes thorough setup and assertions, properly validating the expected outcomes after invoking CheckTx and DeliverTx, and checking the state changes for sender and recipient subaccount balances.
29-333: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-330]
No breaking changes have been introduced to the existing API, as there are no alterations to the signatures of exported functions, global data structures, global variables, interfaces, return values, or thrown exceptions.
Changelist
add e2e tests for
MsgCreateTransfer, which transfers between subaccountsTest Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.