fix: ensure we dont attempt to send funds from deposit to delegate after we refund user #1764
Conversation
…e refund user; fixes #1761
WalkthroughThe changes in this pull request focus on the Changes
Assessment against linked issues
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: 2
🧹 Outside diff range and nitpick comments (1)
x/interchainstaking/keeper/receipt_test.go (1)
500-521: Consider adding error case assertions.While the test case correctly verifies the success flag, consider adding assertions to validate any error messages or specific conditions that led to the refund scenario.
success, err := quicksilver.InterchainstakingKeeper.MintAndSendQAsset(ctx, sender, senderAddress, &zone, amount, false, nil) suite.NoError(err) suite.False(success) + // Verify no QAssets were minted + totalSupply := quicksilver.BankKeeper.GetSupply(ctx, zone.LocalDenom) + suite.Equal(sdk.NewCoin(zone.LocalDenom, sdk.ZeroInt()), totalSupply)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/interchainstaking/keeper/receipt.go(6 hunks)x/interchainstaking/keeper/receipt_test.go(7 hunks)
🔇 Additional comments (8)
x/interchainstaking/keeper/receipt_test.go (6)
400-402: LGTM! Test case properly validates the success flag.
The test case correctly verifies both the error and success flag for the 1:1 redemption rate scenario.
426-428: LGTM! Test case properly validates the success flag.
The test case correctly verifies both the error and success flag for the non-1:1 redemption rate scenario, including validation of the correct amount calculation.
452-454: LGTM! Test case properly validates the success flag.
The test case correctly verifies both the error and success flag for the sub-1:1 redemption rate scenario, including validation of the correct amount calculation.
470-470: LGTM! Test case properly validates non-118 zone with mapped account.
The test case correctly sets up a non-118 zone and verifies the success flag when a mapped account is provided.
Also applies to: 480-482
500-521: LGTM! New test case covers the refund scenario.
This new test case specifically addresses the PR objective by verifying that the function returns false when attempting to mint QAssets in a non-118 zone without a mapped account, which is the refund scenario mentioned in the PR description.
545-547: LGTM! Test case properly validates the success flag with transfer.
The test case correctly verifies both the error and success flag for the transfer scenario, including validation of the IBC escrow balance.
x/interchainstaking/keeper/receipt.go (2)
132-141: Avoid proceeding when success is false
The code inside the if success { ... } block for handling delegation and auto-claim assumes that success being true means qAssets were minted and sent. Ensure that this block is not executed when success is false, as it could lead to inconsistent states or errors.
Line range hint 213-274: Ensure consistent handling of success and error in MintAndSendQAsset
The function MintAndSendQAsset now returns a bool indicating success along with an error. Ensure that all return paths correctly represent the operation's outcome:
- Line 215: Returns
falseand an error if the redemption rate is zero. ✅ - Line 232: Returns
falseand the result ofk.SubmitTxwhen refunding assets. Verify thatk.SubmitTxreturns an appropriate error if the refund fails.⚠️ - Line 241: Returns
falseand an error if minting coins fails. ✅ - Line 265: Returns
falseand an error if transferring coins fails. ✅ - Line 274: Returns
trueandnilwhen the operation is successful. ✅
Run the following script to check that k.SubmitTx correctly returns errors:
Consider adding comments to the function signature to clarify the meaning of the success boolean. This will improve code readability and maintainability.
Apply this diff to add a comment:
func (k *Keeper) MintAndSendQAsset(ctx sdk.Context, sender sdk.AccAddress, senderAddress string, zone *types.Zone, assets sdk.Coins, memoRTS bool, mappedAddress sdk.AccAddress) (bool, error) {
+ // Returns:
+ // - success: true if qAssets were minted and sent successfully.
+ // - error: non-nil if an error occurred during the process.
if zone.RedemptionRate.IsZero() {
return false, errors.New("zero redemption rate")
}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1764 +/- ##
==========================================
+ Coverage 61.40% 61.45% +0.05%
==========================================
Files 196 196
Lines 17075 17077 +2
==========================================
+ Hits 10485 10495 +10
+ Misses 5742 5735 -7
+ Partials 848 847 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
1. Summary
Fixes #1761
When we refund the user, ensure we don't attempt to also send a MsgSend to tranfer the same funds to the delegate account.
2.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
MintAndSendQAssetfunction to handle different scenarios more effectively.Tests