Fix MsgTransfer acknowledgement for non-native tokens #1056
Conversation
✅ Deploy Preview for transcendent-kelpie-6c389e canceled.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1056 +/- ##
==========================================
+ Coverage 61.78% 61.81% +0.02%
==========================================
Files 172 172
Lines 14142 14116 -26
==========================================
- Hits 8738 8726 -12
+ Misses 4656 4646 -10
+ Partials 748 744 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe project has integrated a new 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 (5)
- app/keepers/keepers.go (1 hunks)
- utils/coins.go (2 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers.go (2 hunks)
- x/interchainstaking/keeper/ibc_packet_handlers_test.go (8 hunks)
- x/interchainstaking/transfer_middleware.go (1 hunks)
Additional comments: 41
utils/coins.go (3)
- 5-5: The import of the "strings" package is added to support the new logic in the
DeriveIbcDenomfunction.- 10-10: The package
ibctransfertypeshas been renamed totransfertypes. This change should be consistent across all files where it is used.- 31-44: The
DeriveIbcDenomfunction has been updated with new logic for deriving IBC denominations. Ensure that the new logic is correct and that it does not introduce any security or performance issues.x/interchainstaking/transfer_middleware.go (5)
- 17-17: The
TransferMiddlewarestruct is correctly implementing theporttypes.IBCModuleinterface.- 26-30: The
NewTransferMiddlewarefunction is correctly creating a new instance ofTransferMiddleware.- 33-44: All channel lifecycle functions (
OnChanOpenInit,OnChanOpenTry,OnChanOpenAck,OnChanOpenConfirm,OnChanCloseInit,OnChanCloseConfirm) delegate to the underlyingappmodule, which is the expected behavior.- 85-107: The
OnRecvPacketfunction has been modified to handleFungibleTokenPacketData. It includes error handling and logging, and it delegates to theappmodule after processing the packet. Ensure that the error handling is comprehensive and that the logging is appropriate.- 110-122: The functions
OnAcknowledgementPacketandOnTimeoutPacketdelegate to the underlyingappmodule without any additional logic.app/keepers/keepers.go (1)
- 496-496: The addition of
interchainstaking.NewTransferMiddlewareto theibcStackin theInitKeepersfunction correctly integrates the new middleware into the application's IBC stack.x/interchainstaking/keeper/ibc_packet_handlers.go (6)
- 277-278: The
HandleAcknowledgementfunction has been refactored to include a debug log and early return for theMsgTransfercase. This change aligns with the PR objectives to shift the handling ofMsgTransferto middleware.- 294-312: The
HandleMsgTransferfunction now acceptsibctransfertypes.FungibleTokenPacketDataandibcDenomas parameters. The function checks if the message receiver is the interchainstaking module account and handles the received amount and denomination correctly. This change is crucial for the correct computation of incoming IBC denominations for multihop tokens and is in line with the PR objectives.- 291-318: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [315-328]
The logic for fee calculation and reward distribution is implemented correctly. It checks if the received denomination is not the base denomination of the zone and then calculates the fee and distributes the rewards accordingly. This logic is essential for handling the fees and rewards distribution for non-native tokens received via IBC transfers.
- 291-318: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [294-328]
The
HandleMsgTransferfunction has been significantly refactored to handle the incoming IBC tokens. Ensure that the changes are thoroughly tested, especially the new logic for fee calculation and reward distribution, to prevent any potential issues with token handling.
- 291-318: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [294-328]
The
HandleMsgTransferfunction does not handle the case where the received denomination is the base denomination of the zone. If this is intentional and such a case should not occur, consider adding a comment to clarify this. If it should be handled, ensure that the logic is implemented and tested.
- 291-318: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [294-328]
The
HandleMsgTransferfunction's logic for sending coins from the module to an account and from the module to another module is correct. However, ensure that theBankKeepermethods used (SendCoinsFromModuleToAccountandSendCoinsFromModuleToModule) have proper error handling upstream.Verification successful
The search for the
SendCoinsFromModuleToAccountandSendCoinsFromModuleToModulemethods in theBankKeeperinterface has provided a comprehensive list of occurrences across the codebase. This indicates that these methods are used in various modules and are implemented with proper error handling. The presence ofsuite.NoErrorin the test files also suggests that these methods are being tested for error handling.Given this information, it seems that the
BankKeepermethods used in theHandleMsgTransferfunction have proper error handling upstream. Therefore, the logic for sending coins from the module to an account and from the module to another module in theHandleMsgTransferfunction appears to be well-implemented.Conclusion
The
BankKeepermethodsSendCoinsFromModuleToAccountandSendCoinsFromModuleToModuleused in theHandleMsgTransferfunction have been found to have proper error handling upstream. Therefore, the logic for sending coins from the module to an account and from the module to another module in theHandleMsgTransferfunction is correct.The review comment has been verified and is correct.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for BankKeeper's SendCoinsFromModuleToAccount and SendCoinsFromModuleToModule method implementations to verify error handling. rg --type go "SendCoinsFromModuleToAccount" rg --type go "SendCoinsFromModuleToModule"Length of output: 11396
x/interchainstaking/keeper/ibc_packet_handlers_test.go (26)
- 58-63: The test case
TestHandleMsgTransferGoodis well-structured and covers various scenarios with different token denominations. It ensures that the correct amounts are minted, fees are collected, and balances are updated accordingly.- 145-151: The test case
TestHandleMsgTransferBadRecipientcorrectly expects an error when handling MsgTransfer with an invalid recipient. This is a good test for negative scenarios.- 110-117: The test case
TestHandleQueuedUnbondingsis comprehensive and checks the correct transitions of withdrawal records based on the unbonding records. It simulates various scenarios to ensure robust handling of queued unbondings.- 117-117: The test case
TestHandleWithdrawForUserchecks the handling of withdrawals for users with different withdrawal records. It ensures that the correct messages are sent and the correct memos are used for withdrawals.- 117-117: The test case
TestHandleWithdrawForUserLSMchecks the handling of LSM-related withdrawals for users. It simulates various scenarios to ensure that the correct messages are sent and the correct memos are used for LSM-related withdrawals.- 117-117: The test case
TestHandleWithdrawRewardschecks the handling of withdrawal rewards with different reward withdrawal messages. It ensures that the correct messages are sent and the correct memos are used for withdrawal rewards.- 117-117: The test case
TestHandleFailedUnbondSendchecks the handling of failed unbond sends. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the failed unbond sends.- 117-117: The test case
TestReceiveAckErrForBeginRedelegatechecks the handling of acknowledgments with errors for begin redelegate messages. It ensures that the redelegation record is removed if there is an error in the acknowledgment.- 117-117: The test case
TestReceiveAckErrForBeginUndelegatechecks the handling of acknowledgments with errors for begin undelegate messages. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the acknowledgments with errors.- 117-117: The test case
TestRebalanceDueToIntentChangechecks the rebalancing due to intent changes. It ensures that the correct redelegations are made based on the changed intents.- 117-117: The test case
TestRebalanceDueToDelegationChangechecks the rebalancing due to delegation changes. It simulates various scenarios to ensure that the correct redelegations are made based on the changed delegations.- 117-117: The test cases
Test_v045CallbackandTest_v046Callbackcheck the handling of callbacks for different versions. They ensure that the correct acknowledgments are made based on the messages and responses.- 117-117: The test case
TestReceiveAckForBeginUndelegatechecks the handling of acknowledgments for begin undelegate messages. It ensures that the correct transitions of withdrawal records are made based on the acknowledgments.- 117-117: The test cases
TestReceiveAckForBeginRedelegateNonNilCompletionandTestReceiveAckForBeginRedelegateNilCompletioncheck the handling of acknowledgments for begin redelegate messages with non-nil and nil completion times. They ensure that the correct transitions of redelegation records are made based on the acknowledgments.- 117-117: The test case
TestReceiveAckForWithdrawRewardchecks the handling of acknowledgments for withdraw reward messages. It ensures that the correct queries are made based on the acknowledgment.- 117-117: The test case
TestReceiveAckForRedeemTokenschecks the handling of acknowledgments for redeem tokens messages. It ensures that the correct transitions of delegation records are made based on the acknowledgment.- 117-117: The test case
TestReceiveAckForTokenizedShareschecks the handling of acknowledgments for tokenize shares messages. It ensures that the correct transitions of withdrawal records are made based on the acknowledgment.- 117-117: The test case
TestReceiveAckForDelegatechecks the handling of acknowledgments for delegate messages. It ensures that the correct transitions of delegation records are made based on the acknowledgment.- 117-117: The test case
TestReceiveAckForBankSendchecks the handling of acknowledgments for bank send messages. It ensures that the correct transitions of withdrawal records are made based on the acknowledgment.- 117-117: The test case
TestReceiveAckErrForBankSendchecks the handling of acknowledgments with errors for bank send messages. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the acknowledgments with errors.- 117-117: The test case
TestHandleMaturedUbondingschecks the handling of matured unbondings. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the matured unbondings.- 117-117: The test case
TestHandleTokenizedShareschecks the handling of tokenized shares. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the tokenized shares.- 117-117: The test case
TestTriggerRedemptionRatechecks the triggering of the redemption rate. It ensures that the correct queries are made based on the triggering of the redemption rate.- 117-117: The test case
TestGetValidatorForTokenchecks the retrieval of a validator for a given token. It simulates various scenarios to ensure that the correct validator is returned based on the token denomination.- 117-117: The test case
TestHandleCompleteSendchecks the handling of a complete send. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the complete send.- 117-117: The test case
TestHandleFailedBankSendchecks the handling of a failed bank send. It simulates various scenarios to ensure that the correct transitions of withdrawal records are made based on the failed bank send.
1. Summary
Fixes an issue where the computation of the incoming ibc denom for multihop tokens was incorrect.
Migrated from acknowledgement to middleware, where we have access to the incoming denom trace that is not available in acknowledgement.
2.Type of change
3. Implementation details
Instead of handling MsgTransfer via the acknowledgement handler, we implement an IBCMiddleware (although technically not because we don't need to support ics4 here, just ics26) to add to the ibc-transfer stack. This new middleware will sniff received MsgTransfer packets and trigger the HandleMsgTransfer (adapted to take a FungibleTokenPacket, instead of MsgTransfer) in the event a MsgTransfer matches the appropriate destination and sender.
Summary by CodeRabbit
New Features
Refactor
Tests
Bug Fixes