fix: ensure swapCoin functionality works correctly #942
Conversation
WalkthroughThe pull request introduces modifications to coin denomination handling in the Changes
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
types/constant_test.go (2)
55-58: Add test cases for complete coverageWhile the current test verifies the happy path, we should add test cases for:
- Non-LegacyFXDenom coins (should remain unchanged)
- Edge cases like zero amount
- Verify the actual amount value, not just the string representation
func TestSwapCoin(t *testing.T) { coin := sdk.NewCoin(LegacyFXDenom, sdkmath.NewInt(100)) swapCoin := SwapCoin(coin) assert.NotEqual(t, swapCoin.String(), coin.String()) assert.EqualValues(t, sdk.NewCoin(DefaultDenom, sdkmath.NewInt(1)).String(), swapCoin.String()) + + // Test non-LegacyFXDenom coin + otherCoin := sdk.NewCoin(DefaultDenom, sdkmath.NewInt(100)) + swapOtherCoin := SwapCoin(otherCoin) + assert.Equal(t, otherCoin, swapOtherCoin) + + // Test zero amount + zeroCoin := sdk.NewCoin(LegacyFXDenom, sdkmath.NewInt(0)) + swapZeroCoin := SwapCoin(zeroCoin) + assert.Equal(t, DefaultDenom, swapZeroCoin.Denom) + assert.True(t, swapZeroCoin.Amount.IsZero()) }
62-65: Add test cases for mixed denomination scenariosThe current test only verifies swapping a single LegacyFXDenom coin. We should add test cases for:
- Multiple coins with mixed denominations
- Empty coins collection
- Verify the actual amounts, not just the string representation
func TestSwapCoins(t *testing.T) { coins := sdk.NewCoins(sdk.NewCoin(LegacyFXDenom, sdkmath.NewInt(100))) swapCoins := SwapCoins(coins) assert.NotEqual(t, swapCoins.String(), coins.String()) assert.EqualValues(t, sdk.NewCoins(sdk.NewCoin(DefaultDenom, sdkmath.NewInt(1))).String(), swapCoins.String()) + + // Test mixed denominations + mixedCoins := sdk.NewCoins( + sdk.NewCoin(LegacyFXDenom, sdkmath.NewInt(100)), + sdk.NewCoin(DefaultDenom, sdkmath.NewInt(50)), + ) + swapMixedCoins := SwapCoins(mixedCoins) + expected := sdk.NewCoins( + sdk.NewCoin(DefaultDenom, sdkmath.NewInt(1)), + sdk.NewCoin(DefaultDenom, sdkmath.NewInt(50)), + ) + assert.Equal(t, expected, swapMixedCoins) + + // Test empty coins + emptyCoins := sdk.NewCoins() + swapEmptyCoins := SwapCoins(emptyCoins) + assert.True(t, swapEmptyCoins.Empty()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
types/constant.go(1 hunks)types/constant_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (2)
types/constant.go (2)
117-117: LGTM! Proper handling of Add result.The change correctly captures the result of
Addoperation, fixing a potential bug where the result might have been lost.
106-110: Document the migration fromFXDenomtoLegacyFXDenomThe change from
FXDenomtoLegacyFXDenomsuggests a migration path. This change needs to be documented to help users understand:
- Why we're switching to
LegacyFXDenom- The impact on existing coins
- Migration steps for users
Let's check for any remaining usages of
FXDenomthat might need updating:
Summary by CodeRabbit
Bug Fixes
Tests