refactor(ibctransfer): convert FX to apundiai on OnAck error or Timeout #924
Conversation
WalkthroughThis pull request introduces enhancements to the upgrade process and testing framework for the IBC transfer module. The changes focus on improving token migration logic, specifically for handling escrow denominations during upgrades. New functions are added to validate the state of IBC transfer tokens, manage denomination changes, and centralize packet data unmarshalling. The modifications aim to provide more robust testing and migration capabilities for cross-chain token transfers. Changes
Sequence DiagramsequenceDiagram
participant Upgrade as Upgrade Process
participant TransferModule as IBC Transfer Module
participant EscrowKeeper as Escrow Keeper
Upgrade->>TransferModule: Initiate Migration
TransferModule->>EscrowKeeper: Get Old Denominations
EscrowKeeper-->>TransferModule: Return Escrow Denominations
TransferModule->>TransferModule: Migrate Token Escrow
TransferModule->>EscrowKeeper: Update Escrow Amounts
TransferModule-->>Upgrade: Migration Complete
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for oldDenom, newDenom := range escrowDenoms { | ||
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | ||
| newAmount := totalEscrow.Amount | ||
| if oldDenom == fxtypes.IBCFXDenom { | ||
| newAmount = fxtypes.SwapAmount(newAmount) | ||
| } | ||
| // first remove old denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | ||
| // then add new denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
![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
🧹 Nitpick comments (2)
x/ibc/middleware/keeper/relay_test.go (1)
228-250: Consider handling errors explicitly inmustProtoMarshalJSONThe
mustProtoMarshalJSONhelper function currently panics on errors, which can obscure test failures. Refactoring it to return anerrorallows tests to handle failures gracefully and provides clearer error messages.Apply this refactoring to handle errors without panicking:
-func mustProtoMarshalJSON(msg proto.Message) []byte { +func mustProtoMarshalJSON(msg proto.Message) ([]byte, error) { anyResolver := codectypes.NewInterfaceRegistry() jm := &jsonpb.Marshaler{OrigName: true, EmitDefaults: false, AnyResolver: anyResolver} err := codectypes.UnpackInterfaces(msg, codectypes.ProtoJSONPacker{JSONPBMarshaler: jm}) if err != nil { - panic(err) + return nil, err } buf := new(bytes.Buffer) if err := jm.Marshal(buf, msg); err != nil { - panic(err) + return nil, err } - return buf.Bytes() + return buf.Bytes(), nil }Update the calling code to handle the returned error:
-packetDateByte, err := sdk.SortJSON(mustProtoMarshalJSON(&tcData)) +dataBytes, err := mustProtoMarshalJSON(&tcData) +require.NoError(t, err) +packetDateByte, err := sdk.SortJSON(dataBytes) require.NoError(t, err)app/upgrades/v8/upgrade.go (1)
752-762: Improve documentation and maintainability.The function would benefit from better documentation and use of constants.
+// escrowDenomsMapSize represents the number of denomination mappings +const escrowDenomsMapSize = 2 + +// GetMigrateEscrowDenoms returns a mapping of old denominations to new denominations +// for escrow migration. It handles both: +// 1. FX to PUNDIAI conversion +// 2. PUNDIX unwrapped to wrapped conversion func GetMigrateEscrowDenoms(chainID string) map[string]string { - result := make(map[string]string, 2) + result := make(map[string]string, escrowDenomsMapSize) result[fxtypes.IBCFXDenom] = fxtypes.DefaultDenom pundixDenom := fxtypes.MainnetPundixUnWrapDenom if chainID == fxtypes.TestnetChainId { pundixDenom = fxtypes.TestnetPundixUnWrapDenom } result[pundixDenom] = fxtypes.PundixWrapDenom return result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/upgrade_test.go(2 hunks)app/upgrades/v8/upgrade.go(3 hunks)x/ibc/middleware/keeper/relay.go(2 hunks)x/ibc/middleware/keeper/relay_test.go(3 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
app/upgrades/v8/upgrade.go
[warning] 224-234: Iteration over map
Iteration over map may be a possible source of non-determinism
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (5)
x/ibc/middleware/keeper/relay.go (3)
90-96: Verify the handling of zero amounts inOnAcknowledgementPacketThe updated
OnAcknowledgementPacketmethod introduces logic to handle packets with zero amounts. Ensure that invokingAfterIBCAckSuccesswhenisZeroAmountistruealigns with the intended protocol behavior and doesn't introduce any unintended side effects.
112-118: Confirm consistent handling of zero amounts inOnTimeoutPacketSimilar to
OnAcknowledgementPacket, theOnTimeoutPacketmethod checks forisZeroAmountand callsAfterIBCAckSuccess. Verify that this logic is appropriate for timeout scenarios and that it maintains consistency across different packet handling pathways.
126-148: Ensure robust error handling and correct amount calculations inUnmarshalAckPacketDataThe new
UnmarshalAckPacketDatafunction adds clarity by centralizing the unmarshalling and processing of packet data. Please verify the following:
- Unmarshalling Safety: Confirm that
transfertypes.ModuleCdc.UnmarshalJSONcorrectly handles all possible packet data inputs without risking panics due to malformed data.- Amount and Fee Parsing: Ensure that
parseAmountAndFeeByPacketproperly validates and handles edge cases, such as negative amounts or fees, which could lead to incorrect calculations.- Denomination Swap Logic: Validate that the denomination swap from
fxtypes.IBCFXDenomtofxtypes.DefaultDenomand the application offxtypes.SwapAmountcorrectly reflect the intended token transformation.- Zero Amount Check: The condition
!totalAmount.IsPositive()determines if the total amount is zero or negative. Confirm that this logic aligns with protocol specifications and that downstream processes handle zero or negative amounts appropriately.x/ibc/middleware/keeper/relay_test.go (1)
148-250: Excellent addition of comprehensive tests forUnmarshalAckPacketDataThe
TestUnmarshalAckPacketDatafunction thoroughly tests various scenarios, including normal cases, zero amounts, and packets with fees. This enhances the reliability of theUnmarshalAckPacketDatafunction by ensuring it behaves correctly under different conditions.app/upgrade_test.go (1)
194-206: Good addition of IBC transfer module validation in testsThe new
checkIBCTransferModulefunction effectively validates the migration of escrowed tokens during the upgrade. By asserting that old denominations have zero total escrow and new denominations have non-zero total escrow, it ensures that the migration logic works as intended.
|
|
||
| migrateTransferTokenInEscrow(ctx, app.IBCTransferKeeper) | ||
|
|
There was a problem hiding this comment.
Add error handling for transfer token migration.
The migrateTransferTokenInEscrow call should return and handle potential errors like other migrations in this function to maintain consistency and prevent partial migrations.
- migrateTransferTokenInEscrow(ctx, app.IBCTransferKeeper)
+ if err := migrateTransferTokenInEscrow(ctx, app.IBCTransferKeeper); err != nil {
+ return err
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| migrateTransferTokenInEscrow(ctx, app.IBCTransferKeeper) | |
| if err := migrateTransferTokenInEscrow(ctx, app.IBCTransferKeeper); err != nil { | |
| return err | |
| } | |
| func migrateTransferTokenInEscrow(ctx sdk.Context, transferKeeper ibctransferkeeper.Keeper) { | ||
| escrowDenoms := GetMigrateEscrowDenoms(ctx.ChainID()) | ||
| for oldDenom, newDenom := range escrowDenoms { | ||
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | ||
| newAmount := totalEscrow.Amount | ||
| if oldDenom == fxtypes.IBCFXDenom { | ||
| newAmount = fxtypes.SwapAmount(newAmount) | ||
| } | ||
| // first remove old denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | ||
| // then add new denom | ||
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure deterministic processing of escrow denominations.
The current implementation uses map iteration which could be non-deterministic. For consensus-critical operations, we should ensure deterministic processing order.
func migrateTransferTokenInEscrow(ctx sdk.Context, transferKeeper ibctransferkeeper.Keeper) {
escrowDenoms := GetMigrateEscrowDenoms(ctx.ChainID())
- for oldDenom, newDenom := range escrowDenoms {
+ // Get sorted keys for deterministic iteration
+ oldDenoms := make([]string, 0, len(escrowDenoms))
+ for oldDenom := range escrowDenoms {
+ oldDenoms = append(oldDenoms, oldDenom)
+ }
+ sort.Strings(oldDenoms)
+
+ for _, oldDenom := range oldDenoms {
+ newDenom := escrowDenoms[oldDenom]
totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom)
newAmount := totalEscrow.Amount
if oldDenom == fxtypes.IBCFXDenom {
newAmount = fxtypes.SwapAmount(newAmount)
}
+ // Log migration details
+ ctx.Logger().Info("migrating escrow token",
+ "old_denom", oldDenom,
+ "new_denom", newDenom,
+ "old_amount", totalEscrow.Amount,
+ "new_amount", newAmount)
+
+ if !newAmount.IsPositive() {
+ ctx.Logger().Info("skipping zero amount escrow", "denom", oldDenom)
+ continue
+ }
+
// first remove old denom
transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt()))
// then add new denom
transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount))
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func migrateTransferTokenInEscrow(ctx sdk.Context, transferKeeper ibctransferkeeper.Keeper) { | |
| escrowDenoms := GetMigrateEscrowDenoms(ctx.ChainID()) | |
| for oldDenom, newDenom := range escrowDenoms { | |
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | |
| newAmount := totalEscrow.Amount | |
| if oldDenom == fxtypes.IBCFXDenom { | |
| newAmount = fxtypes.SwapAmount(newAmount) | |
| } | |
| // first remove old denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | |
| // then add new denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | |
| } | |
| } | |
| func migrateTransferTokenInEscrow(ctx sdk.Context, transferKeeper ibctransferkeeper.Keeper) { | |
| escrowDenoms := GetMigrateEscrowDenoms(ctx.ChainID()) | |
| // Get sorted keys for deterministic iteration | |
| oldDenoms := make([]string, 0, len(escrowDenoms)) | |
| for oldDenom := range escrowDenoms { | |
| oldDenoms = append(oldDenoms, oldDenom) | |
| } | |
| sort.Strings(oldDenoms) | |
| for _, oldDenom := range oldDenoms { | |
| newDenom := escrowDenoms[oldDenom] | |
| totalEscrow := transferKeeper.GetTotalEscrowForDenom(ctx, oldDenom) | |
| newAmount := totalEscrow.Amount | |
| if oldDenom == fxtypes.IBCFXDenom { | |
| newAmount = fxtypes.SwapAmount(newAmount) | |
| } | |
| // Log migration details | |
| ctx.Logger().Info("migrating escrow token", | |
| "old_denom", oldDenom, | |
| "new_denom", newDenom, | |
| "old_amount", totalEscrow.Amount, | |
| "new_amount", newAmount) | |
| if !newAmount.IsPositive() { | |
| ctx.Logger().Info("skipping zero amount escrow", "denom", oldDenom) | |
| continue | |
| } | |
| // first remove old denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(oldDenom, sdkmath.ZeroInt())) | |
| // then add new denom | |
| transferKeeper.SetTotalEscrowForDenom(ctx, sdk.NewCoin(newDenom, newAmount)) | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 224-234: Iteration over map
Iteration over map may be a possible source of non-determinism
Summary by CodeRabbit
New Features
Tests
Refactor