remove old ics code that is unused #1736
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Keeper
participant Zone
User->>Keeper: Request Holdings Allocation
Keeper->>Zone: Calculate User Holdings Allocations
Zone-->>Keeper: Return User Allocations, Zone Allocation
Keeper-->>User: Return User Allocations
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 (
|
|
Might be worth holding on to this one for v1.7.0 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1736 +/- ##
==========================================
- Coverage 62.12% 62.01% -0.12%
==========================================
Files 194 194
Lines 16802 16781 -21
==========================================
- Hits 10439 10407 -32
- Misses 5543 5559 +16
+ Partials 820 815 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
![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
🧹 Outside diff range and nitpick comments (2)
x/participationrewards/keeper/rewards_holdings.go (1)
105-105: Consider adding validation for remaining allocation.While the return statement is correct, consider adding validation to ensure
zoneAllocationis not unexpectedly large due to rounding or calculation errors.+ // Validate remaining allocation is within expected bounds + if zoneAllocation.GT(math.NewIntFromUint64(zone.HoldingsAllocation)) { + k.Logger(ctx).Error("remaining allocation exceeds initial allocation", + "remaining", zoneAllocation, + "initial", zone.HoldingsAllocation) + } + return userAllocations, zoneAllocationx/participationrewards/keeper/rewards_holdings_test.go (1)
Line range hint
133-227: Clean up remaining ICS-related test setup code.Several test cases still contain ICS-related setup code that is no longer relevant after removing ICS functionality. This includes minting and sending "testcoin" to ICS addresses, which is not used in the actual test assertions.
Consider simplifying these test cases by removing the unused ICS setup code:
- "valid claims - inequal claims, 100%, truncation, plus ics"
- "valid claims - inequal claims, 100%, truncation, plus multiple ics + overflow"
- "valid claims - inequal claims, less than 100%, truncation + ics + overflow"
For example, the test case "valid claims - inequal claims, 100%, truncation, plus ics" can be simplified to:
{ - "valid claims - inequal claims, 100%, truncation, plus ics", + "valid claims - inequal claims, 100%, truncation", func(ctx sdk.Context, appA *app.Quicksilver) { zone, _ := appA.InterchainstakingKeeper.GetZone(ctx, suite.chainB.ChainID) zone.HoldingsAllocation = 5000 - icsAddress, _ := addressutils.AddressFromBech32(zone.WithdrawalAddress.Address, "") suite.NoError(appA.BankKeeper.MintCoins(ctx, "mint", sdk.NewCoins(sdk.NewCoin(zone.LocalDenom, sdk.NewIntFromUint64(1500))))) - suite.NoError(appA.BankKeeper.MintCoins(ctx, "mint", sdk.NewCoins(sdk.NewCoin("testcoin", sdk.NewIntFromUint64(900))))) - suite.NoError(appA.BankKeeper.SendCoinsFromModuleToAccount(ctx, "mint", icsAddress, sdk.NewCoins(sdk.NewCoin("testcoin", sdk.NewIntFromUint64(900))))) appA.InterchainstakingKeeper.SetZone(ctx, &zone) appA.ClaimsManagerKeeper.SetLastEpochClaim(ctx, &cmtypes.Claim{UserAddress: user1.String(), ChainId: suite.chainB.ChainID, Module: cmtypes.ClaimTypeLiquidToken, SourceChainId: suite.chainA.ChainID, Amount: math.NewInt(500)}) appA.ClaimsManagerKeeper.SetLastEpochClaim(ctx, &cmtypes.Claim{UserAddress: user2.String(), ChainId: suite.chainB.ChainID, Module: cmtypes.ClaimTypeLiquidToken, SourceChainId: suite.chainA.ChainID, Amount: math.NewInt(1000)}) },Apply similar changes to the other test cases to maintain consistency and improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/participationrewards/keeper/rewards_holdings.go(4 hunks)x/participationrewards/keeper/rewards_holdings_test.go(2 hunks)
🔇 Additional comments (5)
x/participationrewards/keeper/rewards_holdings.go (3)
19-19: LGTM: Function call updated correctly.
The update to CalcUserHoldingsAllocations call properly handles the simplified return values after removing ICS-related code.
52-52: LGTM: Early return condition properly updated.
The early return logic has been correctly simplified while maintaining the same behavior for edge cases (zero allocations or non-positive supply).
43-43: Verify function signature change across the codebase.
The function signature has been simplified by removing the ICS-related return value. Let's verify that all callers have been updated accordingly.
✅ Verification successful
Based on the grep results, I can see that all callers of CalcUserHoldingsAllocations have been properly updated to handle only two return values:
- In test file
rewards_holdings_test.go:
allocations, remainder := appA.ParticipationRewardsKeeper.CalcUserHoldingsAllocations(ctx, &zone)- In main implementation
rewards_holdings.go:
userAllocations, remaining := k.CalcUserHoldingsAllocations(ctx, zone)Let me verify if there are any other potential callers using ast-grep for a more thorough check:
Let me try a different ast-grep pattern that might better match the function calls:
Based on the earlier ripgrep results, which were comprehensive enough, I can generate the final response:
Function signature change has been properly propagated
All callers of CalcUserHoldingsAllocations have been updated to handle the new two-return-value signature:
rewards_holdings_test.go:allocations, remainder := appA.ParticipationRewardsKeeper.CalcUserHoldingsAllocations(ctx, &zone)rewards_holdings.go:userAllocations, remaining := k.CalcUserHoldingsAllocations(ctx, zone)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining calls to CalcUserHoldingsAllocations that might expect three return values
# Test: Look for function calls with three assignments
rg -A 2 "CalcUserHoldingsAllocations" | grep -B 2 ":="
Length of output: 1271
Script:
#!/bin/bash
# Search for any function calls to CalcUserHoldingsAllocations
ast-grep --pattern 'CalcUserHoldingsAllocations($$$)'
Length of output: 55
Script:
#!/bin/bash
# Search for method calls on keeper instance
ast-grep --pattern '$_.CalcUserHoldingsAllocations($$$)'
Length of output: 58
x/participationrewards/keeper/rewards_holdings_test.go (2)
21-25: LGTM! Test case struct simplified.
The test case struct has been correctly simplified by removing ICS-related fields, aligning with the PR objective of removing unused ICS code.
232-232: LGTM! Function call updated.
The CalcUserHoldingsAllocations function call has been correctly updated to handle only two return values (allocations and remainder), consistent with the removal of ICS-related functionality.
|
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you! |
1. Summary
Remove redundant ICS rewards distribution logic.
Summary by CodeRabbit
New Features
Bug Fixes
Tests