-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ibc): memo fallback address #2442
Conversation
WalkthroughThe updates introduce 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/uibc/uibc.pb.go
is excluded by:!**/*.pb.go
Files selected for processing (5)
- proto/umee/uibc/v1/uibc.proto (1 hunks)
- x/uibc/ics20.go (2 hunks)
- x/uibc/uics20/ibc_module.go (3 hunks)
- x/uibc/uics20/memo_handler.go (4 hunks)
- x/uibc/uics20/memo_handler_test.go (1 hunks)
Additional comments: 4
x/uibc/ics20.go (1)
- 16-19: The
GetMsgs
function is correctly implemented for unpacking messages from theICS20Memo
struct into[]sdk.Msg
. This addition follows the standard approach in the Cosmos SDK and is essential for processing the messages contained within theICS20Memo
.x/uibc/uics20/memo_handler_test.go (1)
- 122-138: The modifications to the
TestMsgMarshalling
function, including the change to usedeserializeMemo
and the addition of new assertions and tests for different deserialization inputs, are well-implemented. These changes ensure comprehensive testing of the deserialization logic, covering scenarios such as successful deserialization, handling of empty input, and improperly formatted input.x/uibc/uics20/memo_handler.go (1)
- 22-63: The changes in
memo_handler.go
, including the renaming and updating of theonRecvPacket
method toonRecvPacketPre
, the refactoring ofdispatchMemoMsgs
, the addition of thevalidateMemoMsg
method, and the update to thedeserializeMemo
method, significantly improve the clarity, maintainability, and functionality of memo handling in the IBC module. These changes are well-implemented and adhere to best practices.x/uibc/uics20/ibc_module.go (1)
- 41-92: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [4-140]
The changes in
ibc_module.go
, including the addition of theencoding/json
import, detailed comments inOnRecvPacket
, the introduction ofMemoHandler
for memo processing, updated logic inOnRecvPacket
for memo processing and potential receiver overwrite, added event emission, and enhanced error logging, significantly improve the functionality, modularity, and observability of the IBC module. These changes are well-implemented and adhere to best practices.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2442 +/- ##
==========================================
- Coverage 75.38% 69.04% -6.35%
==========================================
Files 100 183 +83
Lines 8025 10796 +2771
==========================================
+ Hits 6050 7454 +1404
- Misses 1589 2732 +1143
- Partials 386 610 +224
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utAck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/uibc/uibc.pb.go
is excluded by:!**/*.pb.go
Files selected for processing (3)
- app/app.go (1 hunks)
- proto/umee/uibc/v1/uibc.proto (1 hunks)
- x/uibc/uics20/ibc_module.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- proto/umee/uibc/v1/uibc.proto
- x/uibc/uics20/ibc_module.go
Additional comments: 1
app/app.go (1)
- 597-599: The comment added emphasizes the importance of positioning the
uics20
module as the last middleware in thetransferStack
. This is crucial to ensure that no other middleware manipulates the packet between theuics20
middleware and the transfer app, which could potentially interfere with the execution of transfer hooks. This change aligns with best practices for middleware ordering in IBC transactions, ensuring that memo processing and potential receiver overwrite logic are correctly handled without interference.
Description
then we continue with the transfer and overwrite the original receiver to fallback_addr if
it's defined.
This is because there could be other middlewares that are already executed.
NOTE: I'm not very convinced about the last point - personally I would always revert the transfer if the memo hook structure is correct but we can't handle it (validation problem, message not supported, or hook execution error). What do you think?
Summary by CodeRabbit