refactor(x/protocol): remove Accounts.String() #19815
Conversation
|
Warning Rate Limit Exceeded@JulianToledano has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 16 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. WalkthroughThe primary focus of the changes is to improve address handling within the Changes
Related 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: .coderabbit.yml
Files selected for processing (10)
- x/protocolpool/depinject.go (1 hunks)
- x/protocolpool/keeper/genesis.go (2 hunks)
- x/protocolpool/keeper/grpc_query_test.go (7 hunks)
- x/protocolpool/keeper/keeper.go (7 hunks)
- x/protocolpool/keeper/keeper_test.go (1 hunks)
- x/protocolpool/keeper/msg_server.go (3 hunks)
- x/protocolpool/keeper/msg_server_test.go (48 hunks)
- x/protocolpool/simulation/operations.go (1 hunks)
- x/protocolpool/simulation/proposals.go (2 hunks)
- x/protocolpool/simulation/proposals_test.go (2 hunks)
Additional comments: 20
x/protocolpool/simulation/proposals_test.go (1)
- 32-47: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [22-45]
The changes in
TestProposalMsgsimprove code readability by directly assigningaddressCodecand using it for address conversion. This approach is more straightforward and maintains the test's integrity.x/protocolpool/simulation/proposals.go (1)
- 37-56: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [31-53]
The modifications in
SimulateMsgCommunityPoolSpendenhance address handling by usingcdcfor conversions, which improves consistency and reliability. The error handling and return statements are correctly implemented.x/protocolpool/keeper/genesis.go (1)
- 54-65: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [57-79]
The changes in
ExportGenesisfor converting recipient addresses to strings align with the new address handling strategy. The error handling and iteration logic are correctly implemented.x/protocolpool/simulation/operations.go (1)
- 75-79: The changes in
SimulateMsgFundCommunityPoolfor converting the funder's address to a string improve error handling and maintain the integrity of the simulation.x/protocolpool/keeper/grpc_query_test.go (1)
- 86-100: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [18-97]
The use of
codectestutilfor address encoding and decoding in tests related to budget operations enhances testing capabilities and ensures consistency in address handling.x/protocolpool/keeper/keeper_test.go (1)
- 71-80: The introduction of the
authorityvariable inSetupTestaligns with the new address handling strategy. The error handling and use of this variable in the initialization ofpoolKeeperare correctly implemented.x/protocolpool/keeper/msg_server.go (3)
- 34-34: The direct use of
msg.RecipientAddressinClaimBudgetsimplifies the code and aligns with the new address handling strategy.- 167-167: The direct use of
msg.RecipientAddressinWithdrawContinuousFundaligns with the new address handling strategy. The error handling and return statements are correctly implemented.- 200-200: The direct use of
msg.RecipientAddressinCancelContinuousFundsimplifies the code and aligns with the new address handling strategy.x/protocolpool/keeper/keeper.go (6)
- 116-120: The conversion from
recipientAddrstring to bytes usingk.authKeeper.AddressCodec().StringToBytes(recipientAddr)is correctly implemented. However, consider adding more detailed error handling or logging to provide insights into why an address might be invalid.- 125-127: The error messages returned in the
withdrawContinuousFundfunction are clear and informative. However, consider usingsdkerrors.Wrapffor consistency with Cosmos SDK error handling practices.- 130-130: The check for fund expiry using
cf.Expiry.Before(k.environment.HeaderService.GetHeaderInfo(ctx).Time)is a good practice. It ensures that funds cannot be withdrawn after they have expired.- 265-268: The conversion from bytes to string using
k.authKeeper.AddressCodec().BytesToString(key)within theiterateAndUpdateFundsDistributionfunction is correctly implemented. However, consider adding error handling for the case where the conversion fails, to ensure the function exits early and cleanly.- 318-320: In the
claimFundsfunction, the approach of getting claimable funds and then distributing them from the community pool is logically sound. However, ensure that the error handling is consistent and provides enough context for debugging.- 334-338: The
getClaimableFundsfunction correctly checks for the existence of a budget for the recipient and handles the case where no budget is found. Consider enhancing the error message to include more details about why the budget might not exist.x/protocolpool/keeper/msg_server_test.go (5)
- 11-11: The addition of
codectestutilfromgithub.com/cosmos/cosmos-sdk/codec/testutilis a positive change, as it standardizes the way addresses are handled in tests, improving consistency and maintainability.- 28-29: Using
codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(recipientAddr)for converting byte slice addresses to string format is a good practice for handling addresses in a consistent manner across tests. This approach enhances readability and ensures that address handling aligns with the SDK's standards.- 348-357: The repeated conversion of different recipient addresses to string format using
codectestutil.CodecOptions{}.GetAddressCodec()is correctly implemented. However, consider refactoring this repetitive code into a helper function within the test suite to improve code maintainability and reduce duplication.Consider creating a helper function within the test suite for address conversion to string format to reduce code duplication and improve maintainability.
- 598-601: The conversion of recipient addresses to string format right before making a call to
suite.msgServer.WithdrawContinuousFundis correctly implemented. This ensures that the test aligns with the updated method signatures that expect string addresses. Good attention to detail in ensuring consistency across the test suite.- 901-905: The conversion of recipient addresses to string format in the
TestCancelContinuousFundtest case is correctly implemented, ensuring that the test aligns with the updated method signatures. This consistency in handling addresses as strings is crucial for maintaining the integrity of the test suite.
x/protocolpool/keeper/keeper.go
Outdated
| @@ -322,11 +331,11 @@ func (k Keeper) claimFunds(ctx context.Context, recipient sdk.AccAddress) (amoun | |||
| return amount, nil | |||
| } | |||
|
|
|||
| func (k Keeper) getClaimableFunds(ctx context.Context, recipient sdk.AccAddress) (amount sdk.Coin, err error) { | |||
| func (k Keeper) getClaimableFunds(ctx context.Context, recipient sdk.AccAddress, recipientAddr string) (amount sdk.Coin, err error) { | |||
There was a problem hiding this comment.
This is called from claimFunds, which just converted the string address to sdk.AccAddress. If both are not passed, we just have to convert it again.
I'm not against it, but I don't know how performance can be affected.
There was a problem hiding this comment.
As it doesn't involve complex operations and inconsistencies, I think we could just call it twice to maintain consistency with other functions
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/protocolpool/depinject.go (1 hunks)
- x/protocolpool/keeper/keeper.go (7 hunks)
- x/protocolpool/keeper/msg_server.go (3 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/protocolpool/depinject.go
- x/protocolpool/keeper/keeper.go
- x/protocolpool/keeper/msg_server.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/protocolpool/keeper/keeper.go (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/protocolpool/keeper/keeper.go
x/protocolpool/keeper/keeper.go
Outdated
| } | ||
|
|
||
| return withdrawnAmount, nil | ||
| } | ||
|
|
||
| func (k Keeper) withdrawRecipientFunds(ctx context.Context, recipient sdk.AccAddress) (sdk.Coin, error) { | ||
| func (k Keeper) withdrawRecipientFunds(ctx context.Context, recipient sdk.AccAddress, recipientAddr string) (sdk.Coin, error) { |
There was a problem hiding this comment.
ditto, maintain consistency
Description
ref:
#13140
#7448
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.
I have...
Summary by CodeRabbit