fix(x/protocolpool): withdraw rewards before export genesis #23467
Conversation
|
Warning Rate limit exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 10 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe pull request introduces changes to the genesis state management in the simapp and protocol pool module. The modifications primarily focus on adjusting the module initialization and export processes. In the Changes
Sequence DiagramsequenceDiagram
participant Keeper
participant Context
participant FundsDistribution
participant RecipientFunds
Keeper->>Context: ExportGenesis
Keeper->>FundsDistribution: IterateAndUpdateFundsDistribution
FundsDistribution-->>Keeper: Update Funds
Keeper->>RecipientFunds: Walk and Withdraw Funds
RecipientFunds-->>Keeper: Withdraw Rewards
Keeper->>Keeper: Export Genesis State
Possibly related PRs
Suggested reviewers
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
Documentation and Community
|
julienrbrt
added
the
backport/v0.52.x
![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 (1)
x/protocolpool/keeper/genesis.go (1)
81-83: Consider wrapping errors for better context.The error handling could be improved by wrapping the errors with additional context about the operation being performed.
if err := k.IterateAndUpdateFundsDistribution(ctx); err != nil { - return nil, err + return nil, fmt.Errorf("failed to update funds distribution: %w", err) } ... if _, err := k.withdrawRecipientFunds(ctx, key.Bytes()); err != nil { - return true, err + return true, fmt.Errorf("failed to withdraw funds for recipient %x: %w", key.Bytes(), err) }Also applies to: 87-89
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
simapp/v2/app_config.go(1 hunks)x/protocolpool/keeper/genesis.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
simapp/v2/app_config.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/protocolpool/keeper/genesis.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
📓 Common learnings
Learnt from: likhita-809
PR: cosmos/cosmos-sdk#18471
File: x/protocolpool/keeper/genesis.go:12-51
Timestamp: 2024-11-10T03:53:32.474Z
Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction.
- The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module.
- The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (1)
simapp/v2/app_config.go (1)
176-197: Correct ordering of protocol pool export before bank.The placement of
pooltypes.ModuleNamebeforebanktypes.ModuleNamein ExportGenesis is crucial as it ensures that all protocol pool rewards are withdrawn and reflected in account balances before the bank module exports its state.
| // refresh all funds | ||
| if err := k.IterateAndUpdateFundsDistribution(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // withdraw all rewards before exporting genesis | ||
| if err := k.RecipientFundDistribution.Walk(ctx, nil, func(key sdk.AccAddress, value types.DistributionAmount) (stop bool, err error) { | ||
| if _, err := k.withdrawRecipientFunds(ctx, key.Bytes()); err != nil { | ||
| return true, err | ||
| } | ||
| return false, nil | ||
| }); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
New functionality requires additional test coverage
While ExportGenesis has basic test coverage in genesis_test.go, the new operations need specific test cases:
- Verify funds refresh via
IterateAndUpdateFundsDistribution - Verify rewards withdrawal through
RecipientFundDistribution.Walk
🔗 Analysis chain
Implementation looks good, but needs test coverage.
The added functionality to refresh funds and withdraw rewards before export is well-structured with proper error handling. However, as noted in a previous review, we still need test coverage for the ExportGenesis function.
Let's verify the test coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for ExportGenesis test coverage
rg -l "TestExportGenesis" "x/protocolpool/keeper"
Length of output: 47
Script:
#!/bin/bash
# Find all test files in keeper directory
fd ".*_test.go" "x/protocolpool/keeper"
# Search for any genesis-related test content
rg -l "genesis" "x/protocolpool/keeper" --type go | grep "_test"
# Search for specific genesis-related function calls in test files
rg "Export.*Genesis|Import.*Genesis" "x/protocolpool/keeper" --type go -A 5
Length of output: 1284
|
@julienrbrt your pull request is missing a changelog! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
x/protocolpool/keeper/genesis_test.go (3)
8-8: Reorder imports according to project guidelines.The imports should follow the standard order: standard library, third-party packages, and then project-specific packages.
Apply this ordering:
import ( "time" "cosmossdk.io/math" "cosmossdk.io/x/protocolpool/types" - "go.uber.org/mock/gomock" sdk "github.com/cosmos/cosmos-sdk/types" + "go.uber.org/mock/gomock" )🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
13-15: Consider adding specific test cases for fund transfers.The mock expectations are correctly set up, but they accept any arguments which might be too permissive. Consider adding specific test cases to verify the exact amounts and accounts involved in the transfers.
Example approach:
suite.bankKeeper.EXPECT(). SendCoinsFromModuleToModule( suite.ctx, types.ProtocolPoolDistrAccount, types.StreamAccount, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100))), ). Return(nil)
56-56: LGTM! Consider adding more balance assertions.The zero balance check correctly verifies that all funds are withdrawn during export. Consider adding assertions for:
- Other potential denominations
- The destination of the withdrawn funds
// Example additional assertions: suite.Require().True(exportedGenState.LastBalance.Amount.IsZero(), "all balances should be zero") // Verify funds in destination account
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
x/protocolpool/keeper/genesis_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/protocolpool/keeper/genesis_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
📓 Learnings (1)
📓 Common learnings
Learnt from: likhita-809
PR: cosmos/cosmos-sdk#18471
File: x/protocolpool/keeper/genesis.go:12-51
Timestamp: 2024-11-10T03:53:32.474Z
Learning: - The user `likhita-809` has confirmed the changes suggested in the previous interaction.
- The file in question is `x/protocolpool/keeper/genesis.go` from a Cosmos SDK module.
- The changes involve optimizing the `InitGenesis` function by removing redundant code and ensuring proper handling of start times for budget proposals.
🪛 golangci-lint (1.62.2)
x/protocolpool/keeper/genesis_test.go
8-8: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
(cherry picked from commit f40f820) # Conflicts: # simapp/app_config.go
…23467) (#23471) Co-authored-by: Julien Robert <[email protected]>
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Improvements
Changes
bankv2module from initial genesis configuration