emit transfer event on vault allocation, retrieval, and withdrawal #2303
Conversation
WalkthroughThe changes across multiple files enhance the validation and error handling for the Changes
Possibly related PRs
Suggested labels
Poem
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (19)
Files not reviewed due to no reviewable changes (1)
Files skipped from review as they are similar to previous changes (13)
Additional comments not posted (32)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- protocol/x/vault/keeper/msg_server_allocate_to_vault.go (3 hunks)
- protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go (1 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault.go (3 hunks)
- protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go (1 hunks)
- protocol/x/vault/keeper/withdraw.go (3 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
- protocol/x/vault/types/expected_keepers.go (0 hunks)
Files not reviewed due to no reviewable changes (1)
- protocol/x/vault/types/expected_keepers.go
Additional comments not posted (10)
protocol/x/vault/keeper/msg_server_retrieve_from_vault.go (1)
19-25: LGTM!The changes introduce a validation check for the
QuoteQuantumsfield, ensuring that only positive values are processed. This enhances the robustness of the function by preventing invalid operations with non-positive amounts.The error handling for the validation check is appropriate, using
errorsmod.Wrapfto provide context and the custom error typetypes.ErrInvalidQuoteQuantums.The updated transfer mechanism improves code clarity by using a structured
Transferobject, encapsulating the sender, recipient, asset ID, and amount. The amount in theTransferobject is guaranteed to be positive due to the earlier validation check, ensuring that the transfer operation is safe and valid.Also applies to: 45-52
protocol/x/vault/keeper/msg_server_allocate_to_vault.go (3)
20-26: LGTM!The validation check for
QuoteQuantumsis a good addition. It ensures that only positive values are processed and returns a descriptive error message for invalid input. This improves the robustness of theAllocateToVaultfunction.
11-11: LGTM!The import of the
sendingtypespackage is necessary to support the usage of theTransfertype in the updated transfer method.
65-72: Verify the impact of the updated transfer method.The change from
subaccountsKeeper.TransferFundsFromSubaccountToSubaccounttosendingKeeper.ProcessTransferimproves the clarity and organization of fund transfers. TheTransfertype encapsulates the transfer logic within a structured format, making it easier to manage.However, please ensure that this change is consistently applied across the codebase and does not introduce any unintended side effects.
Run the following script to verify the usage of the updated transfer method:
Verification successful
To gather more information about the usage of the old transfer method and understand why it's still present, let's run the following script:
The new
ProcessTransfermethod is an abstraction layer, not a replacement.The change from
subaccountsKeeper.TransferFundsFromSubaccountToSubaccounttosendingKeeper.ProcessTransferis not a complete refactoring, but rather an addition of an abstraction layer in the sending module. Here's a breakdown of the current structure:
- The
TransferFundsFromSubaccountToSubaccountmethod is still a core part of the subaccounts module and is used directly in various parts of the codebase.- The new
ProcessTransfermethod in the sending module acts as a wrapper around the old method, adding functionality like telemetry and event generation.- This abstraction allows for centralized processing of transfers in the sending module, which can include additional logic, checks, or side effects.
The presence of both methods is intentional and serves different purposes in the architecture. The vault module now uses the higher-level
ProcessTransfermethod, which provides a more feature-rich transfer process.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the updated transfer method across the codebase. # Test: Search for the usage of the old transfer method. Expect: No occurrences. rg --type go -A 5 $'subaccountsKeeper.TransferFundsFromSubaccountToSubaccount' # Test: Search for the usage of the new transfer method. Expect: Consistent usage across the codebase. rg --type go -A 5 $'sendingKeeper.ProcessTransfer'Length of output: 2892
Script:
#!/bin/bash # Description: Investigate the usage of the old transfer method # Check the content of the file where the old method is present echo "Content of protocol/x/sending/keeper/transfer.go:" cat protocol/x/sending/keeper/transfer.go # Look for any other occurrences of the old method being called echo "\nOther occurrences of TransferFundsFromSubaccountToSubaccount:" rg --type go "TransferFundsFromSubaccountToSubaccount" -C 5Length of output: 15258
protocol/x/vault/types/errors.go (1)
148-152: LGTM!The new error variable
ErrInvalidQuoteQuantumsis registered correctly with the module name, a unique error code, and a clear error message. This addition enhances the error handling capabilities of the module by providing a specific error for cases where theQuoteQuantumsvalue is non-positive.Please ensure that the error code 29 is unique within the module and doesn't conflict with any existing error codes.
protocol/x/vault/keeper/msg_server_allocate_to_vault_test.go (2)
70-80: LGTM!The test case correctly verifies the error handling for allocating zero quantums to a vault. It sets up the necessary preconditions, such as the operator authority and vault balances, and expects the appropriate error message. This addition enhances the robustness of the test suite by ensuring that the system correctly handles invalid input for quantum allocation.
81-91: LGTM!The test case correctly verifies the error handling for allocating negative quantums to a vault. It sets up the necessary preconditions, such as the operator authority and vault balances, and expects the appropriate error message. This addition further enhances the robustness of the test suite by ensuring that the system correctly handles invalid input for quantum allocation.
protocol/x/vault/keeper/msg_server_retrieve_from_vault_test.go (2)
62-75: LGTM!This test case correctly verifies that attempting to retrieve zero quantums from a vault returns an error indicating invalid quote quantums. It's an important edge case to cover.
76-89: Looks good!This test case correctly verifies that attempting to retrieve negative quantums from a vault returns an error indicating invalid quote quantums. It's another important edge case to cover alongside the zero quantums case.
protocol/x/vault/keeper/withdraw.go (1)
229-240: Verify if skipping vaults with non-positivequantumsToTransferis expectedThe code skips vaults where
quantumsToTransferis zero or negative, logging an informational message. Verify if this behavior is expected. IfquantumsToTransfershould always be positive, encountering a zero or negative value might indicate an issue elsewhere in the computation. Consider logging a warning or error to aid in debugging unexpected cases.
tqin7
force-pushed
the
tq/use-sending
branch
from
a9f83a8 to
c9feec3
Compare
tqin7
force-pushed
the
tq/use-sending
branch
from
c9feec3 to
8c24dca
Compare
Changelist
[Describe or list the changes made in this PR]
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.Summary by CodeRabbit
New Features
QuoteQuantumsin vault allocation and retrieval processes.MsgAllocateToVault,MsgRetrieveFromVault, andMsgWithdrawFromMegavault.Bug Fixes
QuoteQuantums, ensuring invalid operations are prevented.Tests
Chores
QuoteQuantums.