refactor(x/ibctransfer): convert osmosis FX to PUNDIAI #901
Conversation
WalkthroughThe pull request introduces a more flexible and generalized approach to handling token denomination wrapping across mainnet and testnet environments. The changes modify the logic for determining when and how token denominations should be wrapped during IBC packet transfers. New constants, maps, and utility functions have been added to Changes
Sequence DiagramsequenceDiagram
participant Sender
participant IBC Middleware
participant Channel Keeper
Sender->>IBC Middleware: Send Packet
IBC Middleware->>IBC Middleware: Check if Denom Needs Wrap
alt Wrap Needed
IBC Middleware->>IBC Middleware: Get Wrapped Denomination
IBC Middleware->>Channel Keeper: Send Packet with Wrapped Denom
else No Wrap Needed
IBC Middleware->>Channel Keeper: Send Packet with Original Denom
end
Possibly related PRs
Poem
Finishing Touches
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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
types/denom_wrap.go (1)
17-23: Consider using environment variables for channel IDs.Channel IDs are currently hardcoded as constants. While this works, it might be better to make these configurable through environment variables or configuration files, especially for values that might change between deployments or environments.
x/ibc/middleware/keeper/wrap_channel_keeper.go (1)
40-42: Consider adding error logging for debugging.While the wrapping logic is correct, adding debug logs would help track denomination transformations during packet sending.
needWrap, wrapDenom := fxtypes.SendPacketDenomNeedWrap(ctx.ChainID(), sourceChannel, packetData.Denom) if needWrap { + ctx.Logger().Debug("wrapping denomination", + "original_denom", packetData.Denom, + "wrapped_denom", wrapDenom, + "channel", sourceChannel) packetData.Denom = wrapDenom }x/ibc/middleware/ibc_middleware.go (1)
61-68: Add transaction metrics for wrapped denominations.Consider adding metrics to track denomination wrapping operations for monitoring and debugging purposes.
needWrap, wrapDenom := fxtypes.OnRecvDenomNeedWrap(chainID, packet.GetDestChannel(), data.Denom) if needWrap { + metrics.IncrCounter(fmt.Sprintf("ibc_denom_wrap_%s", wrapDenom), 1) data.Denom = wrapDenom newAmount, err := fxtypes.OnRecvAmountCovert(wrapDenom, data.Amount) if err != nil { + metrics.IncrCounter("ibc_denom_wrap_errors", 1) return data, err } data.Amount = newAmount }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
types/denom_wrap.go(1 hunks)x/ibc/middleware/ibc_middleware.go(1 hunks)x/ibc/middleware/keeper/wrap_channel_keeper.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (1)
types/denom_wrap.go (1)
26-43: Verify the denomination mapping configuration.The mapping configuration appears correct but could benefit from additional validation:
- Ensure all denominations are properly registered
- Verify that there are no conflicting mappings
- Consider adding validation at startup
✅ Verification successful
Denomination mapping configuration is correctly structured and validated
The configuration shows:
- Proper denomination registration through SDK's RegisterDenom
- No conflicting mappings between networks or within each network's configuration
- Consistent mapping patterns for both Mainnet and Testnet environments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any conflicting or duplicate denomination mappings rg -A 5 "OnRecvWrap.*=.*map\[string\]string{" | grep -E '":.*"' | sort | uniq -d rg -A 5 "SendPacketWrap.*=.*map\[string\]string{" | grep -E '":.*"' | sort | uniq -dLength of output: 445
Script:
#!/bin/bash # Search for denomination map definitions and their contents rg -U "(?s)(Mainnet|Testnet)(OnRecvWrap|SendPacketWrap)\s*=\s*map\[string\]string\{[^}]+\}" # Look for validation or initialization code rg -l "func.*[Ii]nit.*|func.*[Vv]alidate.*" --type go # Search for denomination registration patterns rg "RegisterDenom|SetDenom|AddDenom" --type goLength of output: 6627
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes