feat(uibc): adding ICS 20 memo handler #2349
Conversation
WalkthroughThe updates involve the integration of 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 X ? TipsChat with CodeRabbit Bot (
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2349 +/- ##
==========================================
- Coverage 75.38% 69.56% -5.83%
==========================================
Files 100 172 +72
Lines 8025 12759 +4734
==========================================
+ Hits 6050 8876 +2826
- Misses 1589 3277 +1688
- Partials 386 606 +220
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/uibc/uibc.pb.go
Files selected for processing (8)
- proto/umee/uibc/v1/uibc.proto (1 hunks)
- tests/tsdk/codec.go (1 hunks)
- x/uibc/ics20.go (1 hunks)
- x/uibc/quota/ibc_middleware.go (2 hunks)
- x/uibc/quota/quota.go (3 hunks)
- x/uibc/uics20/ibc_module.go (5 hunks)
- x/uibc/uics20/ics4_module_int_test.go (1 hunks)
- x/uibc/uics20/ics4_wrapper_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
- tests/tsdk/codec.go
Additional comments: 16
proto/umee/uibc/v1/uibc.proto (1)
- 1-16: The
ICS20Memomessage type is well-defined with appropriate syntax, package declaration, and imports. The comments provide clear documentation on the purpose of themessagesfield.x/uibc/ics20.go (1)
- 8-11: The implementation of the
UnpackInterfacesmethod forICS20Memolooks correct and aligns with the Cosmos SDK's patterns for unpacking interfaces. It's important to ensure that alltypes.Anyfields withinICS20Memoare accounted for in this method.x/uibc/quota/ibc_middleware.go (3)
36-46: The signature of
IBCOnRecvPackethas been correctly updated to include theft ics20types.FungibleTokenPacketDataparameter. Ensure that all invocations of this function across the codebase are updated to pass the new parameter.43-45: The
RecordIBCInflowmethod is called with thepacketandft.Denom,ft.Amountfrom theFungibleTokenPacketData. Ensure that theRecordIBCInflowmethod's signature and implementation are updated to handle these arguments correctly.Verification successful
The output from the shell script confirms that the
RecordIBCInflowmethod inx/uibc/quota/quota.gohas been updated to accept three parameters:packet channeltypes.Packet,denom, andamount string. This matches the new usage of the method in theIBCOnRecvPacketfunction, where it is called withpacket,ft.Denom, andft.Amount.The method's implementation appears to handle these arguments correctly, performing checks and calculations based on the packet information and the denomination and amount from the
FungibleTokenPacketData. The method returns anexported.Acknowledgement, which is consistent with the expected behavior described in the review comment.Based on this information, the changes to the
RecordIBCInflowmethod's signature and implementation are consistent with the new usage in theIBCOnRecvPacketfunction, and no issues have been found in this specific context.* 33-46: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [15-45]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the `RecordIBCInflow` method definition to ensure it matches the new usage. ast-grep --lang go --pattern $'func ($_ Keeper) RecordIBCInflow($$$) $$$'Length of output: 2885
Review the entire
ibc_middleware.gofile to ensure that theRecordIBCInflowfunction and related changes are consistent with the PR's objectives, especially regarding import paths, argument names, logic, variable usage, and error handling.Verification successful
The output from the shell script provides the definition and contents of the
RecordIBCInflowfunction in thequota.gofile. The function takes three parameters:packetof typechanneltypes.Packet,denom, andamountboth of typestring. It returns anexported.Acknowledgement.The function's logic includes:
- Checking if the sender chain is the source for the IBC packet and if not, it prefixes the denomination.
- It then tries to get token settings from the leverage module using the IBC denomination.
- If the token is not registered, it skips recording the inflow.
- If there's an error fetching the token settings, it logs the error and returns an error acknowledgment.
- It fetches the exchange rate from the oracle module using the symbol denomination.
- It calculates the inflow in USD and updates the token inflow and inflow sum accordingly.
This function's behavior seems to be consistent with the PR's objectives, which include updates to logic, variable usage, and error handling in the
RecordIBCInflowfunction as part of enhancing the IBC functionality with the ability to handle memos.Based on the provided information, there are no inconsistencies with the PR's objectives regarding the
RecordIBCInflowfunction. The changes to the function seem to align with the intent to update the IBC functionality, particularly in how it records inflows, which could be related to the handling of memos in token transfers.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the `RecordIBCInflow` function definition and usage to ensure it is consistent with the PR's objectives. ast-grep --lang go --pattern $'func ($_ Keeper) RecordIBCInflow($$$) $$$'Length of output: 2885
x/uibc/quota/quota.go (6)
9-9: The import path change from
transfer/typestoics20/typesaligns with the PR's objective to handle ICS 20 memos.229-253: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [232-260]
The removal of the
isSourceChainparameter from theRecordIBCInflowfunction signature is a significant change. Ensure that all calls to this function have been updated to reflect this change, and that the new logic within the function correctly determines the source chain.Verification successful
The shell script execution did not produce any output for both the old and new function signatures. This could mean that there are no instances of the old function signature left in the codebase, and the new function signature is not being used yet, or it could be an issue with the script itself.
Given that the script did not return any results, I cannot confirm whether all calls to the
RecordIBCInflowfunction have been updated to reflect the removal of theisSourceChainparameter. To proceed, I will generate a new shell script to search for any calls toRecordIBCInflowthroughout the codebase. This will help determine if the function is being called with the correct number of arguments.
The new shell script output shows that the
RecordIBCInflowfunction is defined inx/uibc/quota/quota.gowith the updated signature that takes three parameters. Additionally, it is being called inx/uibc/quota/ibc_middleware.gowith three arguments, which matches the new signature.This indicates that the function signature change has been correctly reflected in at least one call within the codebase. However, without further context, I cannot confirm that all calls to this function across the entire codebase have been updated accordingly. For a comprehensive verification, a manual review of the entire codebase would be required.
Based on the information available from the script execution, the change to the
RecordIBCInflowfunction signature appears to be correctly implemented in the instances that were found.* 237-250: The new logic within `RecordIBCInflow` to determine the source chain and handle the `ibcDenom` appears to be correct. However, verify that the `SenderChainIsSource` function is used correctly in all contexts and that the error handling is appropriate and consistent with the rest of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the old function signature to find omissions. ast-grep --lang go --pattern $'func (k Keeper) RecordIBCInflow($_, $_, $_, $_) $_ { $$$ }' # Search for the new function signature to validate usage. ast-grep --lang go --pattern $'func (k Keeper) RecordIBCInflow($_, $_, $_) $_ { $$$ }'Length of output: 179
Script:
#!/bin/bash # Search for calls to the RecordIBCInflow function to ensure they match the new signature. rg 'RecordIBCInflow\('Length of output: 218
Verification successful
The usage of
ics20types.SenderChainIsSourcewithinRecordIBCInflowis unique to this function, and there is no indication of incorrect usage. The error handling withchanneltypes.NewErrorAcknowledgementis consistent with other IBC-related code in the codebase.* 239-243: The use of `sourcePrefix` to construct `prefixedDenom` and derive `ibcDenom` is consistent with the PR's objective to handle ICS 20 memos.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of `SenderChainIsSource` to ensure it's being used correctly. ast-grep --lang go --pattern $'ics20types.SenderChainIsSource($_, $_, $_)' # Search for error handling patterns to ensure consistency. rg 'channeltypes.NewErrorAcknowledgement'Length of output: 576
260-260: The calculation of
inflowInUSDusingMustNewDecFromStrand the exchange rate from the oracle seems correct. However, ensure that theMustNewDecFromStrfunction is used safely and that the calculation accurately reflects the intended conversion to USD.247-247: The change in error handling within
RecordIBCInflowto returnnilfor non-registered tokens instead of an error acknowledgement should be verified to ensure it aligns with the intended error handling strategy for the application.Verification successful
Based on the search results,
ErrNotRegisteredTokenis a common error pattern used across various files in the codebase. However, to determine if the change inquota.goto returnnilinstead of an error is consistent with the application's error handling strategy, we should look for other instances where a similar approach is taken.Let's search for instances in
quota.goand related tests where anilreturn is specifically used in the context of non-registered tokens.
The search results confirm that returning
nilin the context of non-registered tokens is a consistent pattern withinquota.go. The change in the PR where theRecordIBCInflowfunction returnsnilinstead of an error acknowledgement for non-registered tokens aligns with this established error handling strategy in the application.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for error handling patterns for non-registered tokens to ensure consistency. rg 'ErrNotRegisteredToken'Length of output: 2447
Script:
#!/bin/bash # Search for instances in 'quota.go' and related tests where a nil return is used in the context of non-registered tokens. rg -A 3 -B 3 'return nil' x/uibc/quota/quota.go rg -A 3 -B 3 'return nil' x/uibc/quota/intest/quota_test.goLength of output: 1634
x/uibc/uics20/ibc_module.go (3)
106-108: The
dispatchMemoMsgsfunction logs an error if there are more than two messages but does not handle the error beyond logging. Confirm if this is the intended behavior or if further error handling is required.98-156: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [140-157]
Verify that the
deserializeFTDataandDeserializeMemoMsgsfunctions do not introduce security vulnerabilities through deserialization and that all error cases are handled appropriately.
- 53-63: The
OnRecvPacketfunction logs an error if the memo cannot be deserialized but continues processing. Confirm if this is the intended behavior or if the transaction should be stopped when the memo is invalid.x/uibc/uics20/ics4_module_int_test.go (1)
- 17-37: The test
TestMsgMarshallingappears to be well-structured and is testing the marshalling and unmarshalling of theICS20Memotype. It's using assertions to validate that the original messages are equal to the messages after serialization and deserialization, which is a good practice for ensuring data integrity during the process. Ensure that this test covers all edge cases and potential error scenarios for a comprehensive validation of the marshalling logic.x/uibc/uics20/ics4_wrapper_test.go (1)
- 12-18: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [3-18]
The import
"gotest.tools/v3/assert"appears to have been reorganized within the import block. Ensure that this reorganization does not introduce any duplicate imports and that"gotest.tools/v3/assert"is still correctly accessible within the test functions.Verification successful
The shell script output confirms that the import
"gotest.tools/v3/assert"appears only once in the filex/uibc/uics20/ics4_wrapper_test.go, which means there are no duplicate imports after the reorganization. This verifies that the import is correctly accessible within the test functions and there are no issues with the import statements as per the changes mentioned.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for duplicate imports of "gotest.tools/v3/assert" in the file. rg 'gotest.tools/v3/assert' x/uibc/uics20/ics4_wrapper_test.go | wc -lLength of output: 72
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- app/app.go (1 hunks)
- x/uibc/uics20/ibc_module.go (4 hunks)
Additional comments: 2
app/app.go (1)
- 581-586: The addition of
leveragekeeper.NewMsgServerImpl(app.LeverageKeeper)as an argument toNewICS20Modulesuggests integration with theLeverageKeeper. Verify that theLeverageKeeperis correctly integrated and that its methods are compatible with the IBC middleware stack.Verification successful
The script did not find any instances of
NewICS20Modulebeing called with four arguments. To ensure that there are no outdated calls toNewICS20Modulethat need to be updated, I will generate a new script to search for calls toNewICS20Modulewith any number of arguments.
The executed scripts did not find any instances of
NewICS20Modulebeing called elsewhere in the codebase. This suggests that the change to include an additional argument inapp/app.gois localized and does not affect other parts of the codebase. The verification process did not reveal any issues with the integration ofleveragekeeper.NewMsgServerImpl(app.LeverageKeeper)in theNewICS20Modulefunction call.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances of `NewICS20Module` to ensure they are updated. rg 'NewICS20Module' --files-with-matches | xargs -L 1 ast-grep --lang go --pattern $'NewICS20Module($_, $_, $_, $_)'Length of output: 115
Script:
#!/bin/bash # Search for any calls to `NewICS20Module` to ensure all instances are updated. rg 'NewICS20Module' --files-with-matches | xargs -L 1 ast-grep --lang go --pattern $'NewICS20Module($$$)'Length of output: 104
x/uibc/uics20/ibc_module.go (1)
- 33-40: The constructor signature for
NewICS20Modulenow correctly includes theleverage ltypes.MsgServerparameter as per the changes described in the PR and the AI-generated summary. This resolves the previous issue raised about the missing parameter.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/uibc/quota/ibc_middleware.go (2 hunks)
- x/uibc/uics20/ibc_module.go (4 hunks)
Additional comments: 2
x/uibc/quota/ibc_middleware.go (2)
- 36-37: The signature of
IBCOnRecvPackethas been modified to include an additional parameter. Verify that all invocations of this function across the codebase have been updated to pass the new argument.Verification successful
To verify that all invocations of the modified
IBCOnRecvPacketfunction have been updated to pass the new argument, I will search for calls to this function throughout the codebase.
The search results indicate that the call to
IBCOnRecvPacketinibc_module.gohas been updated to include the new parameter, matching the modified function signature. There are no instances of the function being called with the old signature. This suggests that the necessary updates have been made in the codebase.* 44-44: Confirm that the `RecordIBCInflow` method's signature has been updated to accept the new arguments and that it is being used correctly in all places.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the old function signature to find any missed updates. ast-grep --lang go --pattern $'func (k Keeper) IBCOnRecvPacket(packet channeltypes.Packet) exported.Acknowledgement { $$$ }' # Search for the new function signature to validate usage. ast-grep --lang go --pattern $'func (k Keeper) IBCOnRecvPacket(ft ics20types.FungibleTokenPacketData, packet channeltypes.Packet) exported.Acknowledgement { $$$ }'Length of output: 1313
Script:
#!/bin/bash # Search for calls to the IBCOnRecvPacket function to ensure they include the new parameter. rg 'IBCOnRecvPacket' --vimgrepLength of output: 290
Verification successful
The
RecordIBCInflowmethod's signature has indeed been updated to accept the new arguments as indicated by the invocation inibc_middleware.go. The method is defined inx/uibc/quota/quota.goand is being used correctly in the place that was checked.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the `RecordIBCInflow` method definition to check its signature. ast-grep --lang go --pattern $'func (k Keeper) RecordIBCInflow(packet channeltypes.Packet, denom string, amount string) error { $$$ }' # Search for the `RecordIBCInflow` method invocation to validate usage. ast-grep --lang go --pattern $'k.RecordIBCInflow($_, $_, $_)'Length of output: 295
Script:
#!/bin/bash # Adjust the pattern to search for the `RecordIBCInflow` method definition without specifying the exact parameters. ast-grep --lang go --pattern $'func (k Keeper) RecordIBCInflow($$$) error { $$$ }'Length of output: 85
Script:
#!/bin/bash # Use ripgrep to search for the `RecordIBCInflow` method definition in the codebase. rg 'func \(k Keeper\) RecordIBCInflow'Length of output: 144
Description
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation