feat(x/protocolpool)!: allow any coins in continuous funds #21916
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve significant updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yml ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)api/cosmos/protocolpool/v1/genesis.pulsar.go (1)Pattern 🔇 Additional comments (4)api/cosmos/protocolpool/v1/genesis.pulsar.go (4)Line range hint A past review comment already covers the naming convention issue for field descriptors.
The changes effectively support the PR objective of allowing any coins in continuous funds:
The changes effectively support distribution tracking:
The protobuf type definitions are well-structured:
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
|
…/allow-all-coins-protopool
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| x.Amount = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.protocolpool.v1.DistributionAmount")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.protocolpool.v1.DistributionAmount")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.protocolpool.v1.DistributionAmount does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
facundomedica
requested review from
hieuvubk,
alpe and
a team
as code owners
|
@facundomedica your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (16)
x/protocolpool/keeper/genesis_test.go (3)
35-35: LGTM! Consider adding a constant for the coin denom.The change to use
DistributionAmountwithsdk.NewCoinsaligns well with the PR objective of allowing any coins in continuous funds. This provides more flexibility in representing different coin types.Consider defining a constant for the "stake" denom at the package level to improve maintainability:
const testDenom = "stake"Then use it in the test:
Amount: types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(testDenom, math.NewInt(100)))},
43-43: LGTM! Consider using a variable for consistency.The change to use
DistributionAmountforLastBalanceis consistent with the previous change and aligns with the PR objectives.For better consistency and to make the relationship between the distributed amount and last balance clearer, consider using a variable:
distributionAmount := math.NewInt(100) lastBalanceAmount := math.NewInt(1) gs.Distributions = append(gs.Distributions, &types.Distribution{ Amount: types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(testDenom, distributionAmount))}, Time: &time.Time{}, }) gs.LastBalance = types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(testDenom, lastBalanceAmount))}This makes it easier to adjust the test scenario and clearly shows that the last balance is less than the distribution amount.
52-52: LGTM! Consider using a variable for the expected amount.The updated assertion correctly checks the specific coin amount in the new
LastBalancestructure.To improve clarity and maintain consistency with the earlier suggestions, consider using a variable for the expected amount:
expectedLastBalance := math.OneInt() suite.Require().Equal(expectedLastBalance, exportedGenState.LastBalance.Amount.AmountOf(testDenom))This makes the expected value more explicit and easier to update if needed.
x/protocolpool/types/genesis.go (1)
Line range hint
1-78: Overall assessment: Changes look good, but ensure comprehensive testing.The modifications to the
NewGenesisStatefunction, particularly theLastBalancefield initialization, are consistent with the PR objectives of allowing any coins in continuous funds. The changes adhere to Golang coding standards and the Uber Golang style guide.However, given that this change affects a fundamental structure (
GenesisState), it's crucial to:
- Thoroughly test the impact of this change on all components that interact with
GenesisState.- Update any relevant documentation to reflect the new
LastBalancetype and initialization.- Consider adding or updating unit tests to cover the new
LastBalancebehavior.Consider adding a comment above the
NewGenesisStatefunction explaining the rationale behind initializingLastBalancewith an empty set of coins, as this might not be immediately obvious to other developers.x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (1)
46-54: LGTM: NewDistributionAmountmessage added with appropriate options.The new
DistributionAmountmessage is well-structured and aligns with the PR objective of allowing any coins in continuous funds. The use ofrepeated cosmos.base.v1beta1.Coinwith appropriate Gogoproto and Amino options ensures compatibility and consistent behavior.Consider adding a brief comment above the
amountfield to describe its purpose and any constraints (e.g., "Stores multiple coin types for distribution. Must not be empty."). This would enhance the self-documentation of the proto file.message DistributionAmount { + // Stores multiple coin types for distribution. Must not be empty. repeated cosmos.base.v1beta1.Coin amount = 1 [ (gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", (amino.dont_omitempty) = true, (amino.encoding) = "legacy_coins" ]; }x/protocolpool/keeper/msg_server.go (1)
160-162: LGTM with a minor suggestion for clarity.The initialization of
RecipientFundDistributionwith an empty coin set is correct and aligns with the new flexibility to handle multiple coin types. This change supports the PR objective of allowing any coins in continuous funds.For improved clarity and consistency with Go conventions, consider using a composite literal directly:
err = k.RecipientFundDistribution.Set(ctx, recipient, types.DistributionAmount{Amount: sdk.Coins{}})This minor change eliminates the need for the
NewCoins()function call while achieving the same result.x/protocolpool/keeper/keeper_test.go (2)
143-150: LGTM: Updated type handling in TestIterateAndUpdateFundsDistributionThe changes correctly reflect the transition from
math.Inttotypes.DistributionAmount. The test logic has been appropriately updated to check theAmountfield of the new type.Consider adding a test case for a
DistributionAmountwith multiple coins to ensure the new type is fully exercised.
211-220: LGTM: Updated type handling in TestSetToDistributeThe changes correctly reflect the transition to using
DistributionAmount. The test logic has been appropriately updated to check theAmountfield of the new type.Consider adding a test case with multiple coins in the
DistributionAmountto ensure the new type is fully tested.x/protocolpool/keeper/genesis.go (2)
58-58: Consider renaming fields for clarityIn lines 58 and 60, the code references
distribution.Amount.Amount, which can be confusing due to the repeated use ofAmount. To improve readability and maintainability, consider renaming one of theAmountfields in theDistributionAmountstruct. For example, you could rename the innerAmountfield toCoins, resulting indistribution.Amount.Coins.Apply this diff to rename the field:
-type DistributionAmount struct { - Amount sdk.Coins -} +type DistributionAmount struct { + Coins sdk.Coins +}This change will require updating all usages of
distribution.Amount.Amounttodistribution.Amount.Coins.Also applies to: 60-60
Line range hint
121-127: Avoid variable shadowing of theerrvariable in the closureIn the anonymous function passed to
k.Distributions.Walk, the named return parametererr errorshadows theerrvariable in the outer scope. This can lead to confusion and potential errors. According to the Uber Go Style Guide, it's recommended to avoid shadowing variables.Consider removing the named return parameters or renaming them to prevent shadowing.
Apply this diff:
-err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (stop bool, err error) { +err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (stop bool, walkErr error) {Since the
errvariable is not used inside the function body, you could also omit the named return parameter:-err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (stop bool, err error) { +err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (bool, error) {This avoids shadowing the outer
errvariable and improves code clarity.x/protocolpool/keeper/keeper.go (1)
153-154: Address the TODO comment regarding allowed denominationsThere is a TODO comment indicating the need to handle cases where the balance does not contain any of the allowed denominations. Please implement this logic to ensure proper functioning and prevent potential issues with unsupported denominations.
Would you like assistance in implementing this check? I can help draft the necessary code or open a GitHub issue to track this task.
x/protocolpool/keeper/msg_server_test.go (2)
486-488: Redundant Setting of Zero Distribution AmountsIn lines 486-488, both
RecipientFundDistributionandDistributionsare set tosdk.NewCoins(), which represents an empty coin set. Since these are default zero values, consider removing these explicit initializations to reduce redundancy.
Line range hint
832-847: Optimize Redundant Initializations in Test SetupIn lines 832-847,
RecipientFundDistributionis set tosdk.NewCoins()for multiple recipients during test setup. Since the distributions default to empty, these explicit initializations might be unnecessary. Removing them could simplify the test setup.api/cosmos/protocolpool/v1/types.pulsar.go (1)
1370-1370: Avoid leading underscores and underscores in type names; prefer CamelCase.The type
_DistributionAmount_1_listbegins with an underscore and uses underscores in its name, which goes against Go naming conventions. According to the Uber Go Style Guide, underscores should be avoided in names. Consider renaming the type todistributionAmountListorDistributionAmountList.Apply this diff to rename the type:
-type _DistributionAmount_1_list struct { +type distributionAmountList struct { // fields... }Ensure all references to this type are updated accordingly.
api/cosmos/protocolpool/v1/tx.pulsar.go (2)
7467-7469: Fix the indentation of thewithdrawn_allocated_fundfield.The
withdrawn_allocated_fundfield is not properly indented. It should be aligned with the other fields in theMsgCancelContinuousFundResponsemessage.Apply this diff to fix the indentation:
- WithdrawnAllocatedFund []*v1beta1.Coin `protobuf:"bytes,4,rep,name=withdrawn_allocated_fund,json=withdrawnAllocatedFund,proto3" json:"withdrawn_allocated_fund,omitempty"` + WithdrawnAllocatedFund []*v1beta1.Coin `protobuf:"bytes,4,rep,name=withdrawn_allocated_fund,json=withdrawnAllocatedFund,proto3" json:"withdrawn_allocated_fund,omitempty"`
7561-7563: Fix the indentation of theamountfield.The
amountfield is not properly indented. It should be aligned with the other fields in theMsgWithdrawContinuousFundResponsemessage.Apply this diff to fix the indentation:
- Amount []*v1beta1.Coin `protobuf:"bytes,1,rep,name=amount,proto3" json:"amount,omitempty"` + Amount []*v1beta1.Coin `protobuf:"bytes,1,rep,name=amount,proto3" json:"amount,omitempty"`
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
🔇 Files ignored due to path filters (3)
x/protocolpool/types/genesis.pb.gois excluded by!**/*.pb.gox/protocolpool/types/tx.pb.gois excluded by!**/*.pb.gox/protocolpool/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
- api/cosmos/protocolpool/v1/genesis.pulsar.go (32 hunks)
- api/cosmos/protocolpool/v1/tx.pulsar.go (26 hunks)
- api/cosmos/protocolpool/v1/types.pulsar.go (6 hunks)
- x/protocolpool/depinject.go (1 hunks)
- x/protocolpool/keeper/genesis.go (2 hunks)
- x/protocolpool/keeper/genesis_test.go (2 hunks)
- x/protocolpool/keeper/keeper.go (10 hunks)
- x/protocolpool/keeper/keeper_test.go (4 hunks)
- x/protocolpool/keeper/msg_server.go (1 hunks)
- x/protocolpool/keeper/msg_server_test.go (18 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/genesis.proto (2 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto (2 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (2 hunks)
- x/protocolpool/testutil/expected_keepers_mocks.go (0 hunks)
- x/protocolpool/types/expected_keepers.go (0 hunks)
- x/protocolpool/types/genesis.go (1 hunks)
💤 Files not reviewed due to no reviewable changes (2)
- x/protocolpool/testutil/expected_keepers_mocks.go
- x/protocolpool/types/expected_keepers.go
🧰 Additional context used
📓 Path-based instructions (11)
api/cosmos/protocolpool/v1/genesis.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/protocolpool/v1/tx.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/protocolpool/v1/types.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/depinject.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.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"x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper_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"x/protocolpool/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/msg_server_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"x/protocolpool/types/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/protocolpool/keeper/genesis.go (1)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z 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.
🔇 Additional comments not posted (64)
x/protocolpool/keeper/genesis_test.go (1)
Line range hint
1-52: Overall, the changes effectively test the new multi-coin functionality.The updates to
TestInitGenesisalign well with the PR objective of allowing any coins in continuous funds. The test now usesDistributionAmountwithsdk.NewCoinsfor bothDistributionsandLastBalance, which provides more flexibility in representing different coin types.The test scenario effectively checks the case where the total to be distributed is greater than the last balance, and the assertion has been updated to check the specific coin amount in the new structure.
These changes ensure that the genesis functionality is properly tested with the new multi-coin support.
x/protocolpool/types/genesis.go (2)
9-10: LGTM: Import changes are appropriate and well-structured.The addition of the SDK types import is consistent with the changes in the
NewGenesisStatefunction. The import statement follows the Uber Golang style guide recommendations for import grouping and aliasing.
17-17: LGTM:LastBalanceinitialization updated correctly, but verify impact.The change from
math.ZeroInt()toDistributionAmount{Amount: sdk.NewCoins()}forLastBalanceinitialization is correct and aligns with the PR objectives of allowing any coins in continuous funds. This modification reflects the transition from a simple integer to a more flexible coin-based structure.To ensure this change doesn't introduce any issues, please run the following verification:
This will help identify any other parts of the codebase that might need to be updated to accommodate this change.
x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (2)
11-11: LGTM: Import statement for Amino codec added.The addition of the Amino import is appropriate for maintaining backwards compatibility in the Cosmos SDK ecosystem. This import will allow the use of Amino-specific annotations in the protocol definitions.
Line range hint
14-54: Verify integration ofDistributionAmountwith existing messages.While the new
DistributionAmountmessage allows for multiple coin types, the existingBudgetandContinuousFundmessages still use single-coin fields (e.g.,claimed_amount,budget_per_tranche). This might lead to inconsistencies in how funds are handled across different parts of the protocol.Please review and confirm:
- Whether
BudgetandContinuousFundmessages should be updated to useDistributionAmountfor consistency.- How
DistributionAmountwill be integrated with existing functionality.To assist in this verification, you can run the following script to check for potential inconsistencies:
This script will help identify areas where the new
DistributionAmountmight need to be integrated or where existing single-coin usage might need to be reviewed.x/protocolpool/depinject.go (1)
61-61: LGTM. Verify impact on dependent modules.The removal of
StakingKeeperfrom theNewKeeperfunction call aligns with the changes in theModuleInputsstruct. This modification appears intentional and consistent with the module's new design.However, this is a breaking change that may affect other parts of the system relying on the old
Keeperstructure.To ensure this change doesn't introduce unintended side effects, please run the following script to check for any remaining references to
StakingKeeperin theprotocolpoolmodule:Additionally, verify that all dependent modules have been updated to accommodate this change in the
Keeperstructure.✅ Verification successful
Change Verified: No remaining references to
StakingKeeperinx/protocolpool.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for remaining references to StakingKeeper in the protocolpool module # Test: Search for StakingKeeper references in the protocolpool module rg --type go 'StakingKeeper' x/protocolpoolLength of output: 43
Script:
#!/bin/bash # Description: Search the entire codebase for references to StakingKeeper rg --type go 'StakingKeeper'Length of output: 43919
x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto (3)
158-160: Approved: Enhanced flexibility for withdrawn fundsThe change from a single coin to a repeated field of coins for
withdrawn_allocated_fundaligns well with the PR objective. This modification allows for multiple types of coins to be withdrawn when canceling a continuous fund, increasing the flexibility of the protocol.Note that this is a breaking change, as mentioned in the PR title, and will require updates to any code interacting with this message.
172-174: Approved: Multi-coin support for withdrawalsThe modification of the
amountfield to a repeated field of coins inMsgWithdrawContinuousFundResponseis consistent with the PR's goal. This change enables the withdrawal of multiple types of coins from a continuous fund, enhancing the protocol's flexibility.As with the previous change, this is a breaking change that will require updates to dependent code.
Line range hint
1-176: Verify related code updatesThe changes in this file successfully implement multi-coin support for continuous funds in both cancellation and withdrawal operations. These modifications align well with the PR objectives and are correctly implemented in the Protocol Buffers definitions.
However, as these are breaking changes, it's crucial to ensure that:
- The corresponding Go code generated from these Proto definitions is updated accordingly.
- All parts of the codebase that interact with
MsgCancelContinuousFundResponseandMsgWithdrawContinuousFundResponseare updated to handle multiple coins.- Tests are updated or added to cover these new multi-coin scenarios.
- Documentation is updated to reflect these changes.
To verify the impact of these changes, you can run the following script:
This script will help identify areas of the codebase that might need to be updated in light of these changes.
x/protocolpool/keeper/keeper_test.go (3)
38-42: LGTM: Removal of stakingKeeper fieldThe removal of the
stakingKeeperfield from theKeeperTestSuitestruct is consistent with the changes described in the AI summary. This modification streamlines the test suite by removing unnecessary dependencies.
Line range hint
231-235: LGTM: Consistent type update in TestSetToDistributeThe change to use
types.DistributionAmountin theWalkfunction is consistent with the previous modifications. The test logic remains valid and continues to provide good coverage.
Line range hint
1-238: Overall assessment: Changes are well-implemented and sufficiently testedThe modifications in this file consistently reflect the transition from using
math.Inttotypes.DistributionAmountfor handling distribution amounts. The test coverage has been appropriately updated to accommodate these changes, and the overall structure of the tests remains sound.Key points:
- Removal of
stakingKeeperdependency aligns with the PR objectives.- Test logic has been correctly updated to work with the new
DistributionAmounttype.- Existing test coverage appears sufficient for the changes made.
Suggestions for improvement:
- Consider adding test cases that exercise
DistributionAmountwith multiple coins to ensure comprehensive coverage of the new type's capabilities.The changes adhere to Golang style guidelines and maintain the overall quality of the test suite.
x/protocolpool/keeper/keeper.go (16)
24-25: LGTM!The definitions of
authKeeperandbankKeeperin theKeeperstruct are appropriate and follow standard conventions.
36-38: LGTM!The addition of
RecipientFundDistribution,Distributions, andLastBalancefields to theKeeperstruct is correctly implemented, and their types are appropriately defined using thecollectionspackage.
70-72: LGTM!The initialization of the new collections maps and items in the
NewKeeperfunction aligns with the existing code patterns and is correctly implemented.
165-168: LGTM!The calculation of
amountToDistributeusingSafeSuband the handling of potential negative results are correctly implemented.
183-188: LGTM!The logic for transferring funds to the community pool when there are no continuous funds, and resetting
LastBalance, is correctly implemented.
194-194: LGTM!Recording the distribution amount with the current block time is appropriate and correctly implemented.
199-199: LGTM!Updating
LastBalancewith the current balance ensures accurate tracking for future distributions.
221-223: LGTM!Initialization of variables
toDistribute,poolFunds, andfullAmountToDistributeis properly set up for subsequent calculations.
225-225: LGTM!Iterating over
k.Distributionsto calculate allocations is correctly implemented.
266-266: LGTM!Resetting
LastBalanceafter clearing distributions ensures consistent state for future calculations.
271-271: LGTM!Transferring
streamAmtto the stream account is correctly handled.
279-279: LGTM!Transferring
poolFundsto the community pool is appropriately implemented.
301-301: LGTM!Initializing
toClaimwith zero coins for new recipients is correct.
307-308: LGTM!Updating the recipient's claimable amount in
RecipientFundDistributionis correctly implemented.
115-135: Verify callers handle new return type ofwithdrawRecipientFundsThe
withdrawRecipientFundsfunction now returnssdk.Coinsinstead ofsdk.Coin. Ensure that all callers of this function have been updated to handle the new return type to avoid potential runtime errors.Run the following script to find all usages of
withdrawRecipientFundsand check for proper handling:#!/bin/bash # Description: Find all calls to `withdrawRecipientFunds` and verify they handle `sdk.Coins`. # Test: Search for function calls to `withdrawRecipientFunds`. Expect: Callers expect `sdk.Coins`. rg --type go 'withdrawRecipientFunds\(' -A 5
45-45: Verify thatstakingKeeperis no longer used in the codebaseThe
stakingKeeperparameter has been removed from theNewKeeperfunction signature. Ensure that all references tostakingKeeperhave been removed throughout the codebase to prevent any undefined references or build errors.Run the following script to confirm that
stakingKeeperis not referenced elsewhere:x/protocolpool/keeper/msg_server_test.go (5)
398-398: Correct Update towithdrawnAmountTypeThe variable
withdrawnAmounthas been appropriately updated fromsdk.Cointosdk.Coinsto handle multiple coin types, aligning with the new data structures introduced in the codebase.
Line range hint
426-442: Consistent Initialization ofRecipientFundDistributionThe initialization of
RecipientFundDistributionwithsdk.NewCoins()ensures that the distribution amounts are correctly represented as empty coin sets for multiple coin types. This change maintains consistency across the codebase.
628-631: Ensure Accurate Assertions After Multiple WithdrawalsAt lines 628-631, assertions are made on
toClaim.Amountforrecipient2andrecipient3. Verify that these assertions correctly reflect the expected state after withdrawals, and ensure that funds are accurately distributed among all recipients.
991-1007: Correct Handling of Expired Continuous FundsIn the
TestWithdrawExpiredFundsfunction (lines 991-1007), the logic correctly prevents withdrawal of expired funds and handles cancellations without errors. The test accurately verifies that repeated cancellations do not result in errors or unintended fund distribution.
508-508: Confirm Necessity of Empty Expiry in Test CaseIn line 508, the test case sets an empty expiry for the continuous fund. Verify whether an empty expiry is acceptable in the business logic and that it does not introduce unintended behavior.
Run the following script to search for uses of continuous funds without expiry:
api/cosmos/protocolpool/v1/genesis.pulsar.go (25)
265-266: Handling ofLastBalanceinRangemethod is appropriateThe check for
x.LastBalance != niland subsequent processing ensures that theLastBalancefield is correctly included during iteration. This follows standard practice and is compliant with the Uber Go Style Guide.
297-297: Correct implementation ofHasmethod forLastBalanceThe
Hasmethod accurately checks the presence ofLastBalanceby verifying if it is not nil. This is the appropriate way to handle optional fields in Go.
321-321: Proper clearing ofLastBalanceinClearmethodSetting
x.LastBalance = nilcorrectly clears the field, ensuring that subsequent calls recognize it as unset. This adheres to best practices for managing optional fields.
354-354: Accurate retrieval ofLastBalanceinGetmethodThe
Getmethod correctly retrieves theLastBalancefield by returning its proto message representation. This implementation is standard and effective.
390-390: Type assertion inSetmethod is safe and appropriateAssigning
x.LastBalanceafter asserting the value's type to*DistributionAmountis appropriate here, assuming correct usage of the API. There is no immediate risk of a type assertion panic in this context.
427-431: Correct initialization ofLastBalanceinMutablemethodThe method ensures that
LastBalanceis initialized when nil, which prevents potential nil pointer dereferences when accessing it later. Returning the proto message reflects standard practice.
458-459: Proper creation of newDistributionAmountinNewFieldThe creation of a new
DistributionAmountinstance and returning its proto message is correct and follows the expected pattern for this method.
544-545: Efficient size calculation ofLastBalanceinProtoMethodsThe
Sizefunction accurately includes the size ofLastBalancewhen it is not nil. This contributes to correct serialization behavior.
599-609: Correct marshaling ofLastBalanceThe code properly marshals
LastBalancewhen it is not nil, handling errors appropriately. The implementation aligns with the standard serialization process.
Line range hint
766-796: Accurate unmarshaling ofLastBalanceDuring unmarshaling, the code correctly initializes
LastBalanceif it's nil and unmarshals data into it. Error handling is properly managed, ensuring robustness.
870-870: Initialization offd_Distribution_amountis correctThe field descriptor for
amountin theDistributionmessage is properly declared and initialized, which is essential for reflection operations.Also applies to: 877-877
951-956: Appropriate handling ofAmountinRangemethod ofDistributionThe method correctly checks if
x.Amountis not nil and processes it accordingly during iteration. This conforms to best practices.
974-975: Correct implementation ofHasmethod forAmountThe
Hasmethod properly determines the presence of theAmountfield by checking for nil, which is appropriate for optional fields.
994-995: Proper clearing ofAmountinClearmethodSetting
x.Amount = nileffectively clears theAmountfield, ensuring it's unset in subsequent operations.
1040-1041: Safe type assertion inSetmethod forAmountThe assignment to
x.Amountwith type assertion to*DistributionAmountis appropriate and assumes correct usage of proto reflection.
1068-1071: Proper initialization ofAmountinMutablemethodEnsuring
x.Amountis initialized when nil prevents nil pointer exceptions and is a correct approach in theMutablemethod.
1088-1090: Correct instantiation inNewFieldmethodCreating a new
DistributionAmountinstance and returning its proto message aligns with standard implementation for creating new fields.
1164-1167: Accurate size calculation ofAmountIncluding the size of
x.Amountin the total size when not nil is essential for accurate serialization.
1197-1198: Proper marshaling ofAmountThe code marshals
x.Amountcorrectly when it is present, handling potential errors suitably.
1211-1223: Correct marshaling ofTimeinDistributionMarshaling of
x.Timeis handled appropriately, ensuring that the timestamp is correctly serialized.
Line range hint
1274-1342: Robust unmarshaling logic inDistributionThe unmarshaling code properly handles both
TimeandAmountfields, initializing them when necessary and managing errors effectively.
1406-1406: Definition ofLastBalancefield is appropriateChanging
LastBalanceto be of type*DistributionAmountallows for richer data representation and aligns with the intended functionality.
1447-1451: Correct implementation ofGetLastBalancemethodThe getter method accurately returns
x.LastBalanceand handles the case wherexis nil, ensuring reliability.
1466-1467: Proper definition ofTimeandAmountfields inDistributionDefining
TimeandAmountas pointers allows for optional fields and aligns with protobuf conventions.
1490-1499: Correct getter methods forDistributionfieldsThe
GetTimeandGetAmountmethods correctly return their respective fields, handling nil cases appropriately.api/cosmos/protocolpool/v1/tx.pulsar.go (6)
5511-5515: Verify the cardinality ofwithdrawn_allocated_fund.The
withdrawn_allocated_fundfield is defined as a repeated field. Ensure that it is semantically correct for this field to contain multiple coins. If it should only hold a single coin amount, consider changing it to a non-repeated field.
Line range hint
7511-7515: Verify the cardinality ofwithdrawn_allocated_fund.Similar to the previous comment, ensure that the
withdrawn_allocated_fundfield should indeed be a repeated field semantically. If it should only hold a single coin amount, change it to a non-repeated field.
Line range hint
7584-7588: Verify the cardinality ofamount.The
amountfield is defined as a repeated field. Ensure that it is semantically correct for this field to contain multiple coins. If it should only hold a single coin amount, consider changing it to a non-repeated field.
7728-7730: Fix the indentation of thewithdrawn_allocated_fundfield.The
withdrawn_allocated_fundfield is not properly indented. It should be aligned with the other fields in theMsgCancelContinuousFundResponsemessage.Apply this diff to fix the indentation:
- WithdrawnAllocatedFund []*v1beta1.Coin `protobuf:"bytes,4,rep,name=withdrawn_allocated_fund,json=withdrawnAllocatedFund,proto3" json:"withdrawn_allocated_fund,omitempty"` + WithdrawnAllocatedFund []*v1beta1.Coin `protobuf:"bytes,4,rep,name=withdrawn_allocated_fund,json=withdrawnAllocatedFund,proto3" json:"withdrawn_allocated_fund,omitempty"`
7746-7748: Fix the indentation of theamountfield.The
amountfield is not properly indented. It should be aligned with the other fields in theMsgWithdrawContinuousFundResponsemessage.Apply this diff to fix the indentation:
- Amount []*v1beta1.Coin `protobuf:"bytes,1,rep,name=amount,proto3" json:"amount,omitempty"` + Amount []*v1beta1.Coin `protobuf:"bytes,1,rep,name=amount,proto3" json:"amount,omitempty"`
6465-6514: 🛠️ Refactor suggestionRefactor the internal list type into a reusable utility.
Similar to the previous comment, consider extracting the
_MsgWithdrawContinuousFundResponse_1_listtype and its methods into a reusable utility type to improve code modularity and reduce duplication.
| (gogoproto.customtype) = "cosmossdk.io/math.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| google.protobuf.Timestamp time = 1 [(gogoproto.stdtime) = true]; |
There was a problem hiding this comment.
Altering field numbers in Protobuf messages breaks compatibility
Changing the field number of time from 6 to 1 in the Distribution message can result in data corruption or parsing errors. Field numbers in Protobuf are used to identify fields in the binary format, and modifying them disrupts compatibility with existing data and clients that use the previous numbering.
Instead of renumbering existing fields, add new fields with new field numbers, and, if necessary, deprecate the old fields to maintain backward compatibility.
x/protocolpool/keeper/genesis.go
Outdated
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | ||
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | ||
| } |
There was a problem hiding this comment.
Fix the logical inconsistency in the condition and error message
There's a mismatch between the condition being checked and the error message returned. The condition data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) checks if the last balance amount is greater than the total to be distributed. However, the error message states "total to be distributed is greater than the last balance," which implies the opposite.
To correct this, the condition should check if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) instead.
Apply this diff to fix the condition:
-if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
+if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}This change ensures that the error is returned when the total to be distributed exceeds the last balance, aligning the condition with the error message.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | |
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | |
| } | |
| if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | |
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | |
| } |
There was a problem hiding this comment.
I'm confused a bit about the 1st condition, should return an error when totalToBeDistributed > lastBalance?
There was a problem hiding this comment.
@hieuvubk, you're correct. The error should be returned when totalToBeDistributed is greater than data.LastBalance.Amount. Currently, the condition:
if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}checks if data.LastBalance.Amount is greater than totalToBeDistributed, which is the opposite of the intended logic based on the error message.
To fix this inconsistency, the condition should be:
if totalToBeDistributed.IsAnyGT(data.LastBalance.Amount) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) {
return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed))
}This change ensures that an error is returned when totalToBeDistributed exceeds data.LastBalance.Amount, aligning the condition with the error message and the expected behavior.
/_/\
( •.• )
🚀<
A curious rabbit pondering over the code!
There was a problem hiding this comment.
Why not simply go with fmt.Errorf instead which is typically used in these scenarios? For example:
fmt.Errorf("an amount to be distributed is greater than the last balance: %s > %s", totalToBeDistributed, data.LastBalance.Amount))There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
This was actually for testing, but I think it can be useful if we ever encounter this, I'll modify it.
x/protocolpool/keeper/keeper.go
Outdated
| for _, denom := range amount.Amount.Denoms() { | ||
| am := sdk.NewCoin(denom, f.Percentage.MulInt(amount.Amount.AmountOf(denom)).TruncateInt()) | ||
| toDistribute[f.Recipient] = toDistribute[f.Recipient].Add(am) | ||
| fullAmountToDistribute = fullAmountToDistribute.Add(am) |
There was a problem hiding this comment.
Review the use of TruncateInt() in distribution calculations
When calculating each recipient's share, TruncateInt() is used after multiplying by the percentage. This could lead to loss of fractional amounts. Consider whether this aligns with the expected financial logic, or if using RoundInt() would be more appropriate to minimize discrepancies.
| @@ -595,7 +595,7 @@ func (suite *KeeperTestSuite) TestWithdrawContinuousFund() { | |||
| }, | |||
| recipientAddress: []sdk.AccAddress{recipient, recipient2, recipient3}, | |||
| expErr: false, | |||
| withdrawnAmount: sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(30000)), | |||
| withdrawnAmount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(30000))), | |||
There was a problem hiding this comment.
Inconsistent Handling of Multiple Recipient Addresses
In the test case starting at line 598, recipientAddress is a slice containing multiple addresses, but MsgWithdrawContinuousFund accepts a single RecipientAddress. This could lead to confusion or logic errors. Consider adjusting the test to process each recipient individually or modify the message to handle multiple recipients if intended.
| suite.Require().NoError(err) | ||
| suite.mockStreamFunds(math.NewInt(0)) | ||
| }, | ||
| recipientAddress: []sdk.AccAddress{recipient}, | ||
| expErr: false, | ||
| withdrawnAmount: sdk.NewCoin(sdk.DefaultBondDenom, math.ZeroInt()), | ||
| withdrawnAmount: nil, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify Representation of Zero withdrawnAmount
The withdrawnAmount is set to nil on line 494. To maintain consistency and clarity, consider using sdk.NewCoins() to represent zero amounts instead of nil.
Apply this diff to standardize the representation:
- withdrawnAmount: nil,
+ withdrawnAmount: sdk.NewCoins(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| withdrawnAmount: nil, | |
| withdrawnAmount: sdk.NewCoins(), |
| type _DistributionAmount_1_list struct { | ||
| list *[]*v1beta1.Coin | ||
| } |
There was a problem hiding this comment.
Avoid using pointers to slices; slices are reference types in Go.
In the struct _DistributionAmount_1_list, the field list is declared as *[]*v1beta1.Coin. Since slices in Go are reference types, using a pointer to a slice is unnecessary and adds complexity. Consider changing the field to use a slice directly.
Apply this diff to simplify the struct definition:
type _DistributionAmount_1_list struct {
- list *[]*v1beta1.Coin
+ list []*v1beta1.Coin
}Adjust the methods accordingly to reflect this change.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type _DistributionAmount_1_list struct { | |
| list *[]*v1beta1.Coin | |
| } | |
| type _DistributionAmount_1_list struct { | |
| list []*v1beta1.Coin | |
| } |
| value := &_DistributionAmount_1_list{list: &x.Amount} | ||
| return protoreflect.ValueOfList(value) | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.protocolpool.v1.DistributionAmount")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.protocolpool.v1.DistributionAmount does not contain field %s", fd.FullName())) | ||
| } |
There was a problem hiding this comment.
Avoid using panic for error handling; return errors instead.
Using panic in the Mutable method for handling invalid field descriptors is discouraged. Panics can cause unexpected crashes and are harder to recover from. Instead, modify the method to return an error or handle the situation gracefully without panicking.
Consider refactoring the code as follows:
func (x *fastReflection_DistributionAmount) Mutable(fd protoreflect.FieldDescriptor) protoreflect.Value {
switch fd.FullName() {
case "cosmos.protocolpool.v1.DistributionAmount.amount":
if x.Amount == nil {
x.Amount = []*v1beta1.Coin{}
}
value := &_DistributionAmount_1_list{list: &x.Amount}
return protoreflect.ValueOfList(value)
default:
if fd.IsExtension() {
- panic(fmt.Errorf("proto3 declared messages do not support extensions: %s", fd.FullName()))
+ // Handle the error appropriately without panicking
+ return protoreflect.Value{}
}
- panic(fmt.Errorf("message cosmos.protocolpool.v1.DistributionAmount does not contain field %s", fd.FullName()))
+ // Handle the error appropriately without panicking
+ return protoreflect.Value{}
}
}Adjust the method's return type and callers as necessary to handle the absence of a valid value.
Committable suggestion was skipped due to low confidence.
| var _ protoreflect.List = (*_MsgCancelContinuousFundResponse_4_list)(nil) | ||
|
|
||
| type _MsgCancelContinuousFundResponse_4_list struct { | ||
| list *[]*v1beta1.Coin | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) Len() int { | ||
| if x.list == nil { | ||
| return 0 | ||
| } | ||
| return len(*x.list) | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) Get(i int) protoreflect.Value { | ||
| return protoreflect.ValueOfMessage((*x.list)[i].ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) Set(i int, value protoreflect.Value) { | ||
| valueUnwrapped := value.Message() | ||
| concreteValue := valueUnwrapped.Interface().(*v1beta1.Coin) | ||
| (*x.list)[i] = concreteValue | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) Append(value protoreflect.Value) { | ||
| valueUnwrapped := value.Message() | ||
| concreteValue := valueUnwrapped.Interface().(*v1beta1.Coin) | ||
| *x.list = append(*x.list, concreteValue) | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) AppendMutable() protoreflect.Value { | ||
| v := new(v1beta1.Coin) | ||
| *x.list = append(*x.list, v) | ||
| return protoreflect.ValueOfMessage(v.ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) Truncate(n int) { | ||
| for i := n; i < len(*x.list); i++ { | ||
| (*x.list)[i] = nil | ||
| } | ||
| *x.list = (*x.list)[:n] | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) NewElement() protoreflect.Value { | ||
| v := new(v1beta1.Coin) | ||
| return protoreflect.ValueOfMessage(v.ProtoReflect()) | ||
| } | ||
|
|
||
| func (x *_MsgCancelContinuousFundResponse_4_list) IsValid() bool { | ||
| return x.list != nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the internal list type into a reusable utility.
Consider extracting the _MsgCancelContinuousFundResponse_4_list type and its methods into a reusable utility type. This will make the code more modular and reduce duplication if similar list types are needed elsewhere.
x/protocolpool/keeper/genesis.go
Outdated
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | ||
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | ||
| } |
There was a problem hiding this comment.
I'm confused a bit about the 1st condition, should return an error when totalToBeDistributed > lastBalance?
There was a problem hiding this comment.
Nice work!
I am not very familiar with the module but I added some comments that may be helpful.
Are you planning to add a migration? That would be very helpful for existing chains.
You also mentioned an accept list to prevent spam. How is this going to be implemented and maintained?
x/protocolpool/keeper/genesis.go
Outdated
|
|
||
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { |
There was a problem hiding this comment.
last balance should always be >= total to be distributed.
Simplest thing would be to flip the check: totalToBeDistributed.IsAnyGT(last balance)
This includes the denoms implicite so that you can drop the DenomsSubsetOf check
There was a problem hiding this comment.
You are right about the inverted comparison, but I still need DenomsSubsetOf, see:
// IsAnyGT returns true iff for any denom in coins, the denom is present at a
// greater amount in coinsB.
//
// e.g.
// {2A, 3B}.IsAnyGT{A} = true
// {2A, 3B}.IsAnyGT{5C} = false
// {}.IsAnyGT{5C} = false
// {2A, 3B}.IsAnyGT{} = false
This case {}.IsAnyGT{5C} = false shows that if totalToBeDistributed has 1foo but LastBalance contains 1bar, it will be false.
x/protocolpool/keeper/genesis.go
Outdated
| if data.LastBalance.Amount.IsAnyGT(totalToBeDistributed) || !totalToBeDistributed.DenomsSubsetOf(data.LastBalance.Amount) { | ||
| return errors.New("total to be distributed is greater than the last balance" + fmt.Sprint(data.LastBalance.Amount, totalToBeDistributed)) | ||
| } |
There was a problem hiding this comment.
Why not simply go with fmt.Errorf instead which is typically used in these scenarios? For example:
fmt.Errorf("an amount to be distributed is greater than the last balance: %s > %s", totalToBeDistributed, data.LastBalance.Amount))
x/protocolpool/keeper/keeper.go
Outdated
| remaining := math.LegacyOneDec().Sub(percentageToDistribute).MulInt(amount).RoundInt() | ||
| poolFunds = poolFunds.Add(remaining) | ||
| for _, denom := range amount.Amount.Denoms() { | ||
| remaining := sdk.NewCoin(denom, math.LegacyOneDec().Sub(percentageToDistribute).MulInt(amount.Amount.AmountOf(denom)).TruncateInt()) |
There was a problem hiding this comment.
Similar code was used before but is another round down which has a rest that is not distributed.
Better remaining = amount - distributed amount
There was a problem hiding this comment.
should be fixed now
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (5)
x/protocolpool/keeper/genesis.go (1)
Line range hint
120-125: Wrap error when exporting distributions for better contextWrapping errors with contextual information enhances debuggability and maintains consistency in error handling.
Apply this diff to wrap the error:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to export distributions: %w", err) }x/protocolpool/keeper/keeper.go (4)
37-37: Update the field comment for accuracyThe comment for the
Distributionsfield mentionskey: time.Time, denom | value: amounts, but the key istime.Timeand the value istypes.DistributionAmount. Consider updating the comment to accurately reflect the key and value types.
Line range hint
221-316: RefactorIterateAndUpdateFundsDistributionfor clarityThe
IterateAndUpdateFundsDistributionfunction is lengthy and handles multiple responsibilities, including collecting continuous funds, calculating distributions, updating recipient distributions, and removing expired funds. Consider refactoring this function into smaller, well-named helper functions to improve readability and maintainability.
194-199: Check for errors when setting distributionsEnsure that the error returned by
k.Distributions.Setincludes sufficient context. Currently, the error message is:return fmt.Errorf("error while setting Distributions: %w", err)Consider providing more details about the key or the amount being set to aid in debugging.
253-253: Duplicate percentage checkThe check for
percentageToDistributeexceeding1.0is performed earlier at line 247. Consider refactoring or removing redundant checks to streamline the code.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- x/protocolpool/keeper/genesis.go (2 hunks)
- x/protocolpool/keeper/genesis_test.go (2 hunks)
- x/protocolpool/keeper/keeper.go (10 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
x/protocolpool/keeper/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.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"x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/protocolpool/keeper/genesis.go (2)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2024-10-08T15:31:05.486Z 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.Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z 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.
🔇 Additional comments (4)
x/protocolpool/keeper/genesis.go (2)
57-59: Logic for accumulating total distribution amount is correctThe code correctly initializes
totalToBeDistributedwithsdk.NewCoins()and aggregates each distribution amount appropriately.
67-69: Validation logic for distribution amounts is accurateThe condition properly checks if
totalToBeDistributedexceedsdata.LastBalance.Amountor if there are denominations not present indata.LastBalance.Amount. This ensures that the system does not distribute more than what is available.x/protocolpool/keeper/keeper.go (2)
115-135: Ensure proper handling of multiple coins inwithdrawRecipientFundsThe function
withdrawRecipientFundsnow returnssdk.Coinsinstead ofsdk.Coin, indicating support for multiple coin denominations. Verify that all downstream code and callers of this function properly handle multiple coins to prevent potential issues with coin denomination mismatches.
Line range hint
225-255: Ensure proper distribution whenpercentageToDistributesum is less than 1When the total
percentageToDistributeis less than1.0, the remaining funds are calculated and added topoolFunds. Verify that this logic aligns with the expected financial distribution model, and that all fractions of the funds are accounted for accurately.
| Amount: types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(100)))}, | ||
| Time: &time.Time{}, | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding test cases with multiple denominations
While the test initializes the Distribution with a single coin of denomination "stake", the goal of the PR is to allow any coins in continuous funds. To ensure comprehensive test coverage, please add test cases that include multiple denominations. This will help verify that distributions handle various coin types correctly.
| Time: &time.Time{}, | ||
| }) | ||
|
|
||
| err := suite.poolKeeper.InitGenesis(suite.ctx, gs) | ||
| suite.Require().ErrorContains(err, "total to be distributed is greater than the last balance") | ||
|
|
||
| // Set last balance | ||
| gs.LastBalance = math.NewInt(1) | ||
| gs.LastBalance = types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(101)))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extend LastBalance to include multiple denominations in tests
Currently, gs.LastBalance is set using a single coin with denomination "stake". To fully test the new functionality that permits any coins, consider including multiple denominations in LastBalance. This will ensure that the initialization logic handles a variety of coins appropriately.
| @@ -49,5 +49,5 @@ | |||
| suite.Require().NoError(err) | |||
| suite.Require().Equal(gs.ContinuousFund, exportedGenState.ContinuousFund) | |||
| suite.Require().Equal(gs.Budget, exportedGenState.Budget) | |||
| suite.Require().Equal(math.OneInt(), exportedGenState.LastBalance) | |||
| suite.Require().Equal(math.NewInt(101), exportedGenState.LastBalance.Amount.AmountOf("stake")) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance assertions to cover multiple denominations
The assertion on line 52 checks the amount of "stake" in exportedGenState.LastBalance. To validate that ExportGenesis correctly handles multiple coin denominations, please add assertions for other denominations. This will help confirm that the exported state accurately reflects all types of coins in the LastBalance.
| lastBalance, err := k.LastBalance.Get(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap error when retrieving last balance for better context
When returning errors, it's recommended to wrap them with additional context to aid in debugging and provide clarity.
Apply this diff to wrap the error:
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to get last balance: %w", err)
}Committable suggestion was skipped due to low confidence.
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| if err = k.Distributions.Set(ctx, k.HeaderService.HeaderInfo(ctx).Time, amountToDistribute); err != nil { | ||
| if err = k.Distributions.Set(ctx, k.HeaderService.HeaderInfo(ctx).Time, types.DistributionAmount{Amount: amountToDistribute}); err != nil { |
There was a problem hiding this comment.
Confirm uniqueness of timestamp keys in Distributions
Using k.HeaderService.HeaderInfo(ctx).Time as the key in Distributions.Set may lead to key collisions if multiple distributions occur within the same timestamp granularity. Consider ensuring that the timestamps used as keys are unique or incorporate additional logic to prevent accidental overwrites.
x/protocolpool/keeper/keeper.go
Outdated
| poolFunds = poolFunds.Add(amount.Amount.Sub(fullAmountToDistribute...)...) | ||
| for _, denom := range amount.Amount.Denoms() { | ||
| remaining := sdk.NewCoin(denom, math.LegacyOneDec().Sub(percentageToDistribute).MulInt(amount.Amount.AmountOf(denom)).TruncateInt()) | ||
| poolFunds = poolFunds.Add(remaining) | ||
| } |
There was a problem hiding this comment.
Review calculation of poolFunds to avoid double-counting
In lines 251 and 254, poolFunds is being adjusted twice:
- Line 251 subtracts
fullAmountToDistributefromamount.Amountand adds the result topoolFunds. - Lines 253-255 calculate
remainingand add it topoolFunds.
This may result in double-counting the remaining funds. Consider consolidating the calculations to ensure that poolFunds accurately reflects the undistributed amounts without duplication.
x/protocolpool/keeper/keeper.go
Outdated
| poolFunds = poolFunds.Add(remaining) | ||
| poolFunds = poolFunds.Add(amount.Amount.Sub(fullAmountToDistribute...)...) | ||
| for _, denom := range amount.Amount.Denoms() { | ||
| remaining := sdk.NewCoin(denom, math.LegacyOneDec().Sub(percentageToDistribute).MulInt(amount.Amount.AmountOf(denom)).TruncateInt()) |
There was a problem hiding this comment.
Handle potential negative values in remaining funds calculation
When calculating remaining:
remaining := sdk.NewCoin(denom, math.LegacyOneDec().Sub(percentageToDistribute).MulInt(amount.Amount.AmountOf(denom)).TruncateInt())If percentageToDistribute exceeds 1.0, the subtraction could result in a negative value, leading to incorrect coin amounts. Ensure that percentageToDistribute does not exceed 1.0 before performing this calculation.
| amountToDistribute, anyNegative := currentBalance.SafeSub(lastBalance.Amount...) | ||
| if anyNegative { | ||
| return errors.New("error while calculating the amount to distribute, result can't be negative") | ||
| } |
There was a problem hiding this comment.
Validate SafeSub operation to prevent unexpected errors
The use of SafeSub may produce unexpected results if lastBalance.Amount contains denominations not present in currentBalance. This could cause anyNegative to be true even if overall balances are positive. Consider aligning denominations before subtraction or handling missing denominations explicitly.
| if err := k.bankKeeper.SendCoinsFromModuleToModule(ctx, types.ProtocolPoolDistrAccount, types.ModuleName, amountToDistribute); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if !lastBalance.IsZero() { // only reset if the last balance is not zero (so we leave it at zero/nil) | ||
| return k.LastBalance.Set(ctx, math.ZeroInt()) | ||
| if !lastBalance.Amount.IsZero() { // only reset if the last balance is not zero (so we leave it at zero) | ||
| return k.LastBalance.Set(ctx, types.DistributionAmount{Amount: sdk.NewCoins()}) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve condition nesting in SetToDistribute
The nested if statements can be simplified to enhance readability:
if !hasContinuousFunds {
if err := ...; err != nil {
return err
}
if !lastBalance.Amount.IsZero() {
return k.LastBalance.Set(ctx, types.DistributionAmount{Amount: sdk.NewCoins()})
}
return nil
}Consider flattening the conditions or adding comments to clarify the flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
x/protocolpool/keeper/keeper.go (6)
45-45: Consistent updates to NewKeeper functionThe changes in the NewKeeper function signature and the initialization of collections are consistent with the modifications in the Keeper struct. This ensures proper handling of the new
DistributionAmounttype.Consider adding error handling for the
collections.NewMapandcollections.NewItemcalls to improve robustness.Also applies to: 70-72
115-115: Enhanced flexibility in fund withdrawalThe modifications to
withdrawRecipientFundsfunction, particularly the change to returnsdk.Coins, align well with the PR objective of allowing any coins in continuous funds. The error handling has been improved, and the use ofDistributionAmountis consistent with earlier changes.Consider using a custom error type for the "error while distributing funds" case to improve error handling consistency.
Also applies to: 120-122, 125-127, 131-133, 135-135
149-154: Improved multi-denomination support in SetToDistributeThe changes in the
SetToDistributefunction, particularly the use ofsdk.CoinsandIsZero(), enhance the support for multiple denominations and align with the PR objective. The error handling has been improved, making the code more robust.Consider adding a comment explaining the TODO on line 153 to clarify the intended future improvement.
Also applies to: 158-158, 165-168, 183-188, 194-194, 199-199
221-229: Enhanced multi-denomination support in fund distributionThe modifications to the
IterateAndUpdateFundsDistributionfunction significantly improve the handling of multiple denominations, aligning with the PR objective. The introduction ofamountToDistributeandtotalDistributionprovides clearer tracking of distributed amounts.Consider using
sdk.NewCoins()instead ofsdk.NewCoins()for initializing empty coin sets to potentially improve performance.Also applies to: 239-245
264-264: Improved fund distribution logicThe changes in the fund distribution logic, particularly the use of
IsZero()and the updated handling of multi-denomination amounts, enhance the function's robustness and align with the PR objective.Consider adding error handling for the
k.LastBalance.Setcall on line 264 to improve the overall robustness of the function.Also applies to: 269-270, 275-277
275-279: Consider explicit handling of potential negative valuesWhile the use of
sdk.Coinsmay prevent negative values, consider adding an explicit check to ensurepoolFundsis not negative before sending coins to the module. This would improve the robustness of the function and prevent potential issues if the underlying implementation ofsdk.Coinschanges.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- x/protocolpool/keeper/keeper.go (10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
x/protocolpool/keeper/keeper.go (5)
24-25: Improved flexibility in fund handlingThe changes to the Keeper struct, particularly the use of
types.DistributionAmountforRecipientFundDistribution,Distributions, andLastBalance, align well with the PR objective of allowing any coins in continuous funds. This modification enhances the module's flexibility in handling various token types.Also applies to: 36-38
299-299: Consistent handling of multi-denomination amountsThe changes in the handling of
toClaim, particularly the initialization withsdk.NewCoins()and the use of theAddmethod, ensure consistent and correct handling of multi-denomination amounts. This aligns well with the PR objective and improves the overall robustness of the function.Also applies to: 305-306
275-275: Improved calculation of poolFundsThe calculation of
poolFundshas been updated to usetotalDistribution.Sub(amountToDistribute...), which addresses the potential double-counting issue highlighted in a previous review. This change improves the accuracy of the fund distribution process.
165-168: Improve handling of mismatched denominations in SafeSub operationWhile
SafeSubprevents underflow errors, it may still produce unexpected results iflastBalance.Amountcontains denominations not present incurrentBalance. Consider aligning the denominations ofcurrentBalanceandlastBalance.Amountbefore performing the subtraction, or explicitly handle cases where denominations don't match.
243-243:⚠️ Potential issueReconsider the use of
TruncateInt()in distribution calculationsThe use of
TruncateInt()when calculating each recipient's share could lead to loss of fractional amounts. This may result in discrepancies, especially over time or with large amounts. Consider usingRoundInt()instead, or implementing a more precise distribution mechanism that accounts for and redistributes any remaining fractional amounts.
x/protocolpool/keeper/keeper.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // if the balance does not have any of the allowed denoms, return early // TODO |
There was a problem hiding this comment.
shall this check be implemented now?
x/protocolpool/keeper/keeper.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| currentBalance := k.bankKeeper.GetAllBalances(ctx, moduleAccount.GetAddress()) |
There was a problem hiding this comment.
I guess we get the usual: WhaT iF tHe modUle aCCounT Has ManY DeNOM?
Which is that case will be relatively valid. Any way we can make the claimer pay for this call?
As this is call in begin block maybe not, but what prevents in a token factory enabled chain, to send to the module accounts 1 millions worthless tokens until it slows the chain.
Unless we make ProtocolPoolDistrAccount a blocked module account, like FeeCollector is.
There was a problem hiding this comment.
As this is call in begin block maybe not, but what prevents in a token factory enabled chain, to send to the module accounts 1 millions worthless tokens until it slows the chain.
Yeah, I thought the same, realistically it shouldn't be needed but it could make things go wrong in the long run, so I guess I'll add the whitelist now :/
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (12)
x/protocolpool/types/keys.go (1)
34-34: LGTM. Consider adding a comment for clarity.The addition of
ParamsKeyis consistent with the existing code structure and naming conventions. It maintains the sequential numbering of prefixes, which is good for readability and maintenance.Consider adding a brief comment explaining the purpose of
ParamsKey, similar to the comments for other constants in this file. This would enhance code documentation and maintainability.x/protocolpool/types/genesis.go (3)
17-19: LGTM: Changes toGenesisStateare appropriate.The modifications to
LastBalanceand the addition ofParamsalign with the PR objectives and enhance the flexibility of the protocol pool.However, consider initializing
Paramswith default values for consistency:Params: DefaultParams(),This assumes you have a
DefaultParams()function, which is a common pattern in Cosmos SDK modules.
27-29: LGTM: DefaultParamsinitialization is appropriate.The addition of
Paramswith a defaultEnabledDistributionDenomsof["stake"]is consistent with typical Cosmos SDK behavior and aligns with the potential implementation of a token whitelist.Consider making this default value configurable:
Params: NewParams(DefaultEnabledDistributionDenoms),Where
DefaultEnabledDistributionDenomscould be a package-level variable:var DefaultEnabledDistributionDenoms = []string{"stake"}This approach would enhance flexibility for different deployments or testing scenarios.
Line range hint
33-85: Consider updating validation functions for new fields.While the existing validation logic remains valid, it doesn't cover the new
Paramsfield or the changedLastBalancetype in theGenesisState. Consider adding validation for these:
In
ValidateGenesis, add a check forParams:if err := gs.Params.Validate(); err != nil { return err }Add a
Validatemethod to theParamsstruct:func (p Params) Validate() error { if len(p.EnabledDistributionDenoms) == 0 { return errors.New("EnabledDistributionDenoms cannot be empty") } for _, denom := range p.EnabledDistributionDenoms { if err := sdk.ValidateDenom(denom); err != nil { return err } } return nil }In
ValidateGenesis, add a check forLastBalance:if err := gs.LastBalance.Validate(); err != nil { return err }These additions will ensure the new fields are properly validated during genesis.
Would you like me to implement these changes or create a GitHub issue to track this task?
x/protocolpool/keeper/msg_server.go (1)
228-238: LGTM: Well-structured parameter update methodThe new
UpdateParamsmethod is a well-structured addition that follows the existing patterns in the file. It correctly validates the authority before updating parameters, which is crucial for maintaining security.A minor suggestion for improvement:
Consider wrapping the error returned from
k.Params.Set()to provide more context:if err := k.Params.Set(ctx, msg.Params); err != nil { return nil, fmt.Errorf("failed to set params: %w", err) }This change would make debugging easier by providing more information about where the error occurred.
x/protocolpool/keeper/keeper_test.go (4)
82-86: LGTM: Addition of Params setup in SetupTestThe addition of Params setup in the
SetupTestmethod is a good practice. It ensures that the tests are run with a controlled and known initial configuration.Consider adding a test case that verifies the behavior with multiple enabled distribution denoms to fully test the new functionality allowing any coins in continuous funds.
124-125: LGTM: Updated TestIterateAndUpdateFundsDistributionThe changes in the
TestIterateAndUpdateFundsDistributionmethod, including the use ofGetBalance, singleCoinrepresentation, andDistributionAmount, align well with the new functionality. The test logic effectively verifies the correct distribution of funds.Consider adding test cases with multiple coin types to fully validate the new capability of handling any coins in continuous funds.
Also applies to: 148-155
184-185: LGTM: Updated TestSetToDistributeThe changes in the
TestSetToDistributemethod, including the use ofGetBalance, singleCoinrepresentation, andDistributionAmount, are consistent with the new functionality and previous changes. The test effectively verifies the behavior of theSetToDistributefunction.To enhance test coverage:
- Consider adding test cases with multiple coin types to validate handling of various tokens.
- Add a test case where the balance includes multiple coin denominations to ensure correct handling of the new multi-coin capability.
Also applies to: 188-188, 216-216, 219-220, 225-225, 228-229, 236-236
Line range hint
1-243: Overall: Good updates, consider expanding test coverageThe changes in this test file consistently reflect the new functionality allowing any coins in continuous funds. The updates to use
GetBalanceinstead ofGetAllBalances, singleCoinrepresentation, andDistributionAmountare appropriate and maintain the integrity of the tests.To further improve the test suite:
- Add test cases that use multiple coin types in continuous funds to fully validate the new multi-coin capability.
- Include scenarios where balances and distributions involve multiple coin denominations.
- Consider edge cases, such as distributions with very small percentages or with the maximum allowed number of coin types.
These additions would ensure comprehensive coverage of the new functionality and potential edge cases.
x/protocolpool/keeper/keeper.go (3)
Line range hint
24-39: LGTM: Keeper struct updates improve flexibility and decouplingThe changes to the Keeper struct, including the removal of
stakingKeeperand the use oftypes.DistributionAmount, appear to enhance the module's flexibility in handling different coin denominations while reducing dependencies. This is a positive architectural change.Consider documenting the rationale behind removing the
stakingKeeperdependency, as this might impact other parts of the system that previously relied on this connection.
117-137: LGTM with suggestion: Improve error handling in withdrawRecipientFundsThe function has been updated to handle multi-denomination distributions, which is a good improvement. However, there's room for better error handling.
Consider wrapping the error in line 129 with more context:
- return nil, fmt.Errorf("error while distributing funds: %w", err) + return nil, fmt.Errorf("error while distributing funds to recipient %s: %w", recipient, err)This will provide more detailed information for debugging purposes.
Line range hint
230-317: LGTM with suggestions: IterateAndUpdateFundsDistribution updated for multi-denomination supportThe function has been significantly refactored to support multiple denominations in distributions. This increases flexibility but also complexity.
Consider the following improvements:
- Extract the distribution calculation logic (lines 251-254) into a separate helper function for better readability and testability.
- Add comments explaining the purpose of
amountToDistributeandallDistributionsvariables, as their roles might not be immediately clear.Example refactoring:
func (k Keeper) calculateDistribution(amount types.DistributionAmount, f types.ContinuousFund) sdk.Coins { distribution := sdk.NewCoins() for _, denom := range amount.Amount.Denoms() { am := sdk.NewCoin(denom, f.Percentage.MulInt(amount.Amount.AmountOf(denom)).TruncateInt()) distribution = distribution.Add(am) } return distribution }This would simplify the main function and make the distribution calculation logic easier to understand and test.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
api/cosmos/protocolpool/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/protocolpool/types/genesis.pb.gois excluded by!**/*.pb.gox/protocolpool/types/tx.pb.gois excluded by!**/*.pb.gox/protocolpool/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
- api/cosmos/protocolpool/v1/genesis.pulsar.go (38 hunks)
- api/cosmos/protocolpool/v1/types.pulsar.go (6 hunks)
- x/protocolpool/autocli.go (1 hunks)
- x/protocolpool/keeper/keeper.go (10 hunks)
- x/protocolpool/keeper/keeper_test.go (7 hunks)
- x/protocolpool/keeper/msg_server.go (2 hunks)
- x/protocolpool/keeper/msg_server_test.go (19 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/genesis.proto (1 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto (4 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (2 hunks)
- x/protocolpool/testutil/expected_keepers_mocks.go (1 hunks)
- x/protocolpool/types/expected_keepers.go (1 hunks)
- x/protocolpool/types/genesis.go (1 hunks)
- x/protocolpool/types/keys.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
api/cosmos/protocolpool/v1/genesis.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/protocolpool/v1/types.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/autocli.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper_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"x/protocolpool/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/msg_server_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"x/protocolpool/testutil/expected_keepers_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 buf
x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto
6-6: import "cosmos/protocolpool/v1/types.proto": file does not exist
(COMPILE)
🔇 Additional comments (36)
x/protocolpool/types/genesis.go (1)
9-10: LGTM: Import changes are appropriate.The addition of the
sdkimport is necessary for the new usage ofsdk.NewCoins(). The import follows the Uber Go Style Guide for third-party package aliases.x/protocolpool/keeper/msg_server.go (2)
160-160: LGTM: Improved flexibility for continuous fund distributionThis change aligns well with the PR objective of allowing any coins in continuous funds. By using
types.DistributionAmount{Amount: sdk.NewCoins()}instead ofmath.ZeroInt(), the implementation now supports multiple coin types, enhancing the flexibility of the protocol pool functionality.
Line range hint
1-268: Overall assessment: Changes align with PR objectives and maintain code qualityThe modifications in this file successfully implement the goal of allowing any coins in continuous funds. The new
UpdateParamsmethod adds necessary functionality for parameter updates. Both changes maintain the existing code structure and adhere to the Uber Go Style Guide.Key points:
- The
CreateContinuousFundmethod now supports multiple coin types.- The new
UpdateParamsmethod provides a secure way to update parameters.These changes enhance the flexibility and functionality of the protocol pool without compromising code quality or security.
x/protocolpool/keeper/keeper_test.go (3)
38-42: LGTM: Removal of stakingKeeper from KeeperTestSuiteThe removal of the
stakingKeeperfield from theKeeperTestSuitestruct is consistent with the changes allowing any coins in continuous funds. This simplification of the test suite structure aligns well with the new functionality.
100-101: LGTM: Updated mockWithdrawContinuousFund methodThe changes in the
mockWithdrawContinuousFundmethod, specifically the use ofGetBalanceinstead ofGetAllBalancesand the representation of balance as a singleCoin, align well with the new functionality. This approach is more precise and efficient for testing the withdrawal of continuous funds.
109-110: LGTM: Updated mockStreamFunds methodThe changes in the
mockStreamFundsmethod, including the use ofGetBalanceand the representation of balance as a singleCoin, are consistent with the updates in other parts of the test suite. These changes appropriately reflect the new functionality allowing any coins in continuous funds.x/protocolpool/keeper/keeper.go (7)
Line range hint
46-74: LGTM: NewKeeper function updated consistentlyThe changes in the NewKeeper function correctly reflect the updates made to the Keeper struct. The removal of the
stakingKeeperparameter and the use oftypes.DistributionAmountin initializations are consistent and appropriate.
Line range hint
331-352: LGTM: claimFunds function remains consistentThe
claimFundsfunction has not undergone significant changes and continues to function as expected within the updated context of the module.
Line range hint
354-411: LGTM: getClaimableFunds function remains consistentThe
getClaimableFundsfunction has not undergone significant changes and continues to operate correctly within the updated module context.
Line range hint
413-460: LGTM: calculateClaimableFunds function remains consistentThe
calculateClaimableFundsfunction has not undergone significant changes and continues to operate correctly within the updated module context.
Line range hint
462-501: LGTM: validateAndUpdateBudgetProposal function remains consistentThe
validateAndUpdateBudgetProposalfunction has not undergone significant changes and continues to operate correctly within the updated module context.
Line range hint
503-523: LGTM: validateContinuousFund function remains consistentThe
validateContinuousFundfunction has not undergone significant changes and continues to operate correctly within the updated module context.
Line range hint
147-208: LGTM with considerations: SetToDistribute function updated with whitelist supportThe function has been updated to support whitelisted denominations, which adds flexibility to the system. The balance calculation and distribution logic have been adjusted accordingly.
Consider the following edge cases:
- What happens if
EnabledDistributionDenomsis empty?- How does the system handle a scenario where a previously whitelisted denomination is removed?
To verify these scenarios, you could run the following script:
This will help identify if these edge cases are handled elsewhere in the codebase.
x/protocolpool/proto/cosmos/protocolpool/v1/genesis.proto (4)
20-20: Note: Previous compatibility concern still applies to 'last_balance' fieldChanging the field type of
last_balancefromstringtoDistributionAmountmay cause backward compatibility issues with existing clients and persisted data that expect astring. This could lead to serialization and deserialization problems.
27-27: Addition of 'params' field to 'GenesisState' is appropriateThe new
paramsfield is correctly added with a unique field number5. This ensures backward compatibility and extends the functionality of theGenesisStatemessage.
31-31: Note: Previous compatibility concern still applies to changing 'time' field numberAltering the field number of
timefrom6to1in theDistributionmessage can break backward compatibility. Existing data and clients expecttimeto be associated with field number6.
33-33: Note: Previous compatibility concern still applies to 'amount' fieldUpdating the type of the
amountfield fromstringtoDistributionAmountmay lead to deserialization issues with existing clients and data expecting astring. This can cause runtime errors or data loss when processing distributions.x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (1)
56-61: Ensure proper initialization and integration ofParamsmessageThe new
Paramsmessage introduces theenabled_distribution_denomsfield to whitelist allowed denominations for distribution. Please verify that:
- The
Paramsstruct is properly registered with the parameter store.- Default parameter values are set appropriately.
- Validation logic is implemented to ensure only valid denominations are allowed.
- Any necessary migrations are handled if existing state is affected.
Run the following script to verify the integration:
x/protocolpool/testutil/expected_keepers_mocks.go (1)
132-145: NewGetBalancemethods are correctly implementedThe addition of
GetBalancemethods in bothMockBankKeeperandMockBankKeeperMockRecordercorrectly mocks the base method and follows the existing pattern used in the generated mock file. The implementation conforms to the Uber Go Style Guide.x/protocolpool/keeper/msg_server_test.go (1)
398-398: UpdatewithdrawnAmountto handle multiple coinsThe
withdrawnAmountvariable has been updated tosdk.Coinsto support multiple coin types, enhancing the flexibility of the withdrawal process. Ensure all references and assertions related towithdrawnAmountare updated accordingly throughout the tests.api/cosmos/protocolpool/v1/genesis.pulsar.go (14)
267-268: Check for nil before usingProtoReflect()In the
Rangemethod, you correctly check ifx.LastBalanceis not nil before proceeding. This ensures safe access to its methods.
279-284: Check for nil before usingProtoReflect()In the
Rangemethod forParams, you correctly check ifx.Paramsis not nil before accessing it. This follows safe coding practices.
305-309: Consistent nil checks inHasmethodNil checks for
x.Paramsandx.LastBalanceare correctly implemented in theHasmethod, ensuring the method accurately reports field population.
331-335: Consistent field clearing inClearmethodThe
Clearmethod correctly setsx.Paramsandx.LastBalancetonil, properly clearing the fields.
405-411: Properly setting fields inSetmethodThe
Setmethod correctly assigns the incoming value tox.Params, ensuring that theParamsfield is updated appropriately.
444-459: Nil checks before field mutation inMutablemethodIn the
Mutablemethod, you properly check ifx.Paramsisnilbefore instantiating a newParamsobject. This prevents potential nil pointer dereferences.
480-487: Consistent field initialization inNewFieldmethodThe
NewFieldmethod correctly initializes new instances forLastBalanceandParams, ensuring new fields are created properly.
Line range hint
569-582: Including new fields in size calculationIn the size calculation, you correctly include
x.Paramsandx.LastBalance, ensuring accurate computation of the message size.
612-625: HandlingParamsin marshalingThe
Marshalmethod appropriately handlesx.Params, including nil checks and error handling, ensuring thatParamsis correctly serialized.
642-652: IncludingLastBalancein marshalingYou correctly marshal
x.LastBalancewith proper error handling, ensuring it's serialized when present.
1485-1490: Updating struct fields correctlyIn the
GenesisStatestruct, you've updated theLastBalancefield to use*DistributionAmountand added theParamsfield. This change enhances the data structure's flexibility.
Line range hint
1527-1547: Getter methods should handle nil casesIn the
GetLastBalance()andGetParams()methods, you correctly handle the possibility ofnilvalues by returningnilif the fields are not set.
1553-1554: Correctly updatingDistributionstruct and gettersThe
Distributionstruct now includesAmountas a pointer toDistributionAmount. The corresponding getter methods handlenilchecks appropriately.Also applies to: 1577-1586
1030-1035:⚠️ Potential issueAdd nil check before accessing
x.AmountIn the
Rangemethod forDistribution, ensure thatx.Amountis not nil before callingProtoReflect()to prevent nil pointer dereference.Apply this diff to include a nil check:
if x.Amount != nil { value := protoreflect.ValueOfMessage(x.Amount.ProtoReflect()) if !f(fd_Distribution_amount, value) { return } +} else { + return }Likely invalid or redundant comment.
api/cosmos/protocolpool/v1/types.pulsar.go (2)
1368-1397: LGTMThe implementation of the
_DistributionAmount_1_liststruct and its methods appears correct and follows Go coding conventions.
2495-2502: LGTMThe
DistributionAmountstruct is properly defined, and the fieldAmountis appropriately typed as a slice of*v1beta1.Coin.
| { | ||
| RpcMethod: "UpdateParams", | ||
| Use: "update-params-proposal <params>", | ||
| Short: "Submit a proposal to update protocolpool module params. Note: the entire params must be provided.", | ||
| Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal '{ "enabled_distribution_denoms": ["stake", "foo"] }'`, version.AppName), | ||
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | ||
| GovProposal: true, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance clarity and provide more specific guidance for the UpdateParams command.
The new RPC command for UpdateParams is well-structured and consistent with other commands. However, consider the following improvements:
- Enhance readability of the example by splitting it across multiple lines.
- Provide more specific information in the short description about what parameters can be updated.
- Consider adding a note about parameter validation, if applicable.
Here's a suggested refactor:
{
RpcMethod: "UpdateParams",
Use: "update-params-proposal <params>",
- Short: "Submit a proposal to update protocolpool module params. Note: the entire params must be provided.",
+ Short: "Submit a proposal to update protocolpool module params (e.g., enabled_distribution_denoms). Note: the entire params must be provided.",
- Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal '{ "enabled_distribution_denoms": ["stake", "foo"] }'`, version.AppName),
+ Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal '
+{
+ "enabled_distribution_denoms": ["stake", "foo"]
+}'`, version.AppName),
PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}},
GovProposal: true,
},Consider adding a note about parameter validation if it's handled elsewhere in the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal <params>", | |
| Short: "Submit a proposal to update protocolpool module params. Note: the entire params must be provided.", | |
| Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal '{ "enabled_distribution_denoms": ["stake", "foo"] }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, | |
| { | |
| RpcMethod: "UpdateParams", | |
| Use: "update-params-proposal <params>", | |
| Short: "Submit a proposal to update protocolpool module params (e.g., enabled_distribution_denoms). Note: the entire params must be provided.", | |
| Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal ' | |
| { | |
| "enabled_distribution_denoms": ["stake", "foo"] | |
| }'`, version.AppName), | |
| PositionalArgs: []*autocliv1.PositionalArgDescriptor{{ProtoField: "params"}}, | |
| GovProposal: true, | |
| }, |
| @@ -17,13 +17,10 @@ type AccountKeeper interface { | |||
|
|
|||
| // BankKeeper defines the expected interface needed to retrieve account balances. | |||
| type BankKeeper interface { | |||
| GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin | |||
There was a problem hiding this comment.
Consider removing the 'Get' prefix from the method name.
According to the Uber Go Style Guide, getter methods should avoid using the 'Get' prefix. It's recommended to rename the method GetBalance to Balance for consistency and clarity.
Apply this diff to rename the method:
type BankKeeper interface {
- GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin
+ Balance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin
GetAllBalances(ctx context.Context, addr sdk.AccAddress) sdk.Coins
SpendableCoins(ctx context.Context, addr sdk.AccAddress) sdk.Coins
SendCoinsFromModuleToAccount(ctx context.Context, senderModule string, recipientAddr sdk.AccAddress, amt sdk.Coins) error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GetBalance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin | |
| Balance(ctx context.Context, addr sdk.AccAddress, denom string) sdk.Coin |
| @@ -8,6 +8,7 @@ import "cosmos_proto/cosmos.proto"; | |||
| import "cosmos/base/v1beta1/coin.proto"; | |||
| import "google/protobuf/timestamp.proto"; | |||
| import "google/protobuf/duration.proto"; | |||
| import "amino/amino.proto"; | |||
There was a problem hiding this comment.
Avoid introducing dependency on deprecated Amino serialization
The import of amino/amino.proto reintroduces a dependency on the legacy Amino serialization format, which is being phased out in favor of Protobuf encoding in the Cosmos SDK. It's recommended to avoid using Amino unless absolutely necessary for backward compatibility.
| // DistributionAmount is used to store the coins of periodic distributions. | ||
| message DistributionAmount { | ||
| repeated cosmos.base.v1beta1.Coin amount = 1 [ | ||
| (gogoproto.nullable) = false, | ||
| (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", | ||
| (amino.dont_omitempty) = true, | ||
| (amino.encoding) = "legacy_coins" | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Reevaluate usage of deprecated Amino options in DistributionAmount message
The DistributionAmount message includes Amino-specific options such as (amino.dont_omitempty) and (amino.encoding), which depend on the legacy Amino serialization format. Since the Cosmos SDK is moving away from Amino, consider removing these options to avoid dependencies on deprecated technology.
| @@ -3,6 +3,7 @@ package cosmos.protocolpool.v1; | |||
|
|
|||
| option go_package = "cosmossdk.io/x/protocolpool/types"; | |||
|
|
|||
| import "cosmos/protocolpool/v1/types.proto"; | |||
There was a problem hiding this comment.
Importing Non-existent 'types.proto' File
The import statement at line 6 references cosmos/protocolpool/v1/types.proto, but this file does not exist in the codebase. This will lead to compilation errors.
🧰 Tools
🪛 buf
6-6: import "cosmos/protocolpool/v1/types.proto": file does not exist
(COMPILE)
| @@ -174,6 +174,7 @@ var ( | |||
| fd_GenesisState_budget protoreflect.FieldDescriptor | |||
| fd_GenesisState_last_balance protoreflect.FieldDescriptor | |||
| fd_GenesisState_distributions protoreflect.FieldDescriptor | |||
| fd_GenesisState_params protoreflect.FieldDescriptor | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure field descriptors follow Go naming conventions
The variables fd_GenesisState_params and fd_Distribution_amount use underscores, which is not in line with the Go naming conventions as outlined in the Uber Go Style Guide. Variable names should be in camelCase.
Apply this diff to align with the naming conventions:
- fd_GenesisState_params protoreflect.FieldDescriptor
+ fdGenesisStateParams protoreflect.FieldDescriptor
...
- fd_GenesisState_params = md_GenesisState.Fields().ByName("params")
+ fdGenesisStateParams = md_GenesisState.Fields().ByName("params")Also applies to: 187-187
| case "cosmos.protocolpool.v1.Distribution.amount": | ||
| value := x.Amount | ||
| return protoreflect.ValueOfMessage(value.ProtoReflect()) |
There was a problem hiding this comment.
Potential nil pointer dereference in Get method
Similar to the GenesisState, in the Get method of fastReflection_Distribution, there is no nil check for x.Amount before calling value.ProtoReflect(). This could cause a nil pointer dereference if x.Amount is nil.
Apply this diff to prevent the nil pointer dereference:
case "cosmos.protocolpool.v1.Distribution.amount":
+ if x.Amount == nil {
+ return protoreflect.Value{}
+ }
value := x.Amount
return protoreflect.ValueOfMessage(value.ProtoReflect())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "cosmos.protocolpool.v1.Distribution.amount": | |
| value := x.Amount | |
| return protoreflect.ValueOfMessage(value.ProtoReflect()) | |
| case "cosmos.protocolpool.v1.Distribution.amount": | |
| if x.Amount == nil { | |
| return protoreflect.Value{} | |
| } | |
| value := x.Amount | |
| return protoreflect.ValueOfMessage(value.ProtoReflect()) |
| case "cosmos.protocolpool.v1.GenesisState.params": | ||
| value := x.Params | ||
| return protoreflect.ValueOfMessage(value.ProtoReflect()) |
There was a problem hiding this comment.
Potential nil pointer dereference in Get method
In the Get method of fastReflection_GenesisState, the case for "cosmos.protocolpool.v1.GenesisState.params" does not check if x.Params is nil before calling value.ProtoReflect(). This could result in a nil pointer dereference if x.Params is nil.
Apply this diff to safely handle the nil case:
case "cosmos.protocolpool.v1.GenesisState.params":
+ if x.Params == nil {
+ return protoreflect.Value{}
+ }
value := x.Params
return protoreflect.ValueOfMessage(value.ProtoReflect())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "cosmos.protocolpool.v1.GenesisState.params": | |
| value := x.Params | |
| return protoreflect.ValueOfMessage(value.ProtoReflect()) | |
| case "cosmos.protocolpool.v1.GenesisState.params": | |
| if x.Params == nil { | |
| return protoreflect.Value{} | |
| } | |
| value := x.Params | |
| return protoreflect.ValueOfMessage(value.ProtoReflect()) |
| fd_Distribution_amount protoreflect.FieldDescriptor | ||
| ) | ||
|
|
||
| func init() { | ||
| file_cosmos_protocolpool_v1_genesis_proto_init() | ||
| md_Distribution = File_cosmos_protocolpool_v1_genesis_proto.Messages().ByName("Distribution") | ||
| fd_Distribution_amount = md_Distribution.Fields().ByName("amount") | ||
| fd_Distribution_time = md_Distribution.Fields().ByName("time") | ||
| fd_Distribution_amount = md_Distribution.Fields().ByName("amount") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent naming conventions for field descriptors
The variable fd_Distribution_amount should follow Go naming conventions. Consider renaming it to fdDistributionAmount.
Apply this diff for consistency:
- fd_Distribution_amount protoreflect.FieldDescriptor
+ fdDistributionAmount protoreflect.FieldDescriptor
...
- fd_Distribution_amount = md_Distribution.Fields().ByName("amount")
+ fdDistributionAmount = md_Distribution.Fields().ByName("amount")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fd_Distribution_amount protoreflect.FieldDescriptor | |
| ) | |
| func init() { | |
| file_cosmos_protocolpool_v1_genesis_proto_init() | |
| md_Distribution = File_cosmos_protocolpool_v1_genesis_proto.Messages().ByName("Distribution") | |
| fd_Distribution_amount = md_Distribution.Fields().ByName("amount") | |
| fd_Distribution_time = md_Distribution.Fields().ByName("time") | |
| fd_Distribution_amount = md_Distribution.Fields().ByName("amount") | |
| fdDistributionAmount protoreflect.FieldDescriptor | |
| ) | |
| func init() { | |
| file_cosmos_protocolpool_v1_genesis_proto_init() | |
| md_Distribution = File_cosmos_protocolpool_v1_genesis_proto.Messages().ByName("Distribution") | |
| fd_Distribution_time = md_Distribution.Fields().ByName("time") | |
| fdDistributionAmount = md_Distribution.Fields().ByName("amount") |
| // enabled_distribution_denoms lists the denoms that are allowed to be distributed. | ||
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. | ||
| EnabledDistributionDenoms []string `protobuf:"bytes,1,rep,name=enabled_distribution_denoms,json=enabledDistributionDenoms,proto3" json:"enabled_distribution_denoms,omitempty"` |
There was a problem hiding this comment.
Update field comments to follow Go documentation conventions
The comments for the EnabledDistributionDenoms field should start with the field name and be full sentences, per the Uber Go Style Guide.
Apply this diff to update the comments:
- // enabled_distribution_denoms lists the denoms that are allowed to be distributed.
- // This is to avoid spending time distributing undesired tokens to continuous funds and budgets.
+ // EnabledDistributionDenoms lists the denominations that are allowed to be distributed.
+ // This avoids spending time distributing undesired tokens to continuous funds and budgets.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // enabled_distribution_denoms lists the denoms that are allowed to be distributed. | |
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. | |
| EnabledDistributionDenoms []string `protobuf:"bytes,1,rep,name=enabled_distribution_denoms,json=enabledDistributionDenoms,proto3" json:"enabled_distribution_denoms,omitempty"` | |
| // EnabledDistributionDenoms lists the denominations that are allowed to be distributed. | |
| // This avoids spending time distributing undesired tokens to continuous funds and budgets. | |
| EnabledDistributionDenoms []string `protobuf:"bytes,1,rep,name=enabled_distribution_denoms,json=enabledDistributionDenoms,proto3" json:"enabled_distribution_denoms,omitempty"` |
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (16)
x/protocolpool/types/keys.go (1)
34-34: LGTM. Consider adding a comment for clarity.The addition of
ParamsKeyis consistent with the existing code structure and naming conventions. It follows the sequential numbering pattern for prefixes.For improved clarity, consider adding a brief comment explaining the purpose of
ParamsKey, similar to comments provided for other constants in this file.x/protocolpool/types/genesis.go (3)
17-19: LGTM: GenesisState initialization updatedThe changes to
LastBalanceand the addition ofParamsalign with the PR objective of allowing any coins in continuous funds.However, consider initializing
Paramswith default values instead of an empty struct for consistency withDefaultGenesisState().You might want to update line 19 as follows:
Params: DefaultParams(),This assumes you have a
DefaultParams()function defined elsewhere in the package.
27-29: LGTM with suggestions: DefaultGenesisState updated with ParamsThe addition of
ParamswithEnabledDistributionDenomsaligns with the PR objective of potentially implementing a whitelist of tokens to prevent spam.Consider the following suggestions:
- The default value only includes "stake". Consider if this is sufficiently flexible for different network configurations.
- It might be beneficial to define these default values as constants elsewhere in the package for easier maintenance and reusability.
You might want to define a constant for the default enabled denominations:
const DefaultEnabledDenom = "stake" // In DefaultGenesisState() Params: &Params{ EnabledDistributionDenoms: []string{DefaultEnabledDenom}, },This would make it easier to update the default value in the future if needed.
Update Validation Functions to Include EnabledDistributionDenoms
The validation functions
validateBudgetandvalidateContinuousFunddo not currently validate theEnabledDistributionDenomsfield:
- validateBudget: Add validation to ensure that
EnabledDistributionDenomsinParamsis properly checked.- validateContinuousFund: Implement checks to confirm that the coins used are within the
EnabledDistributionDenomslist.These updates are necessary to ensure that the new
EnabledDistributionDenomsfunctionality is correctly validated during genesis.🔗 Analysis chain
Line range hint
44-85: Consider updating validation functionsThe validation functions
validateBudgetandvalidateContinuousFundremain unchanged. Consider updating these functions to accommodate the new changes:
validateBudget: Add validation for the newParamsfield, particularly theEnabledDistributionDenoms.validateContinuousFund: Consider adding checks to ensure the coins used are in theEnabledDistributionDenomslist.These updates would ensure the new functionality is properly validated during genesis.
To assist in identifying areas that might need updating, you can run the following command:
This will help identify if the new field is being used in validation functions across the package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of EnabledDistributionDenoms in validation functions rg --type go 'func.*validate.*|EnabledDistributionDenoms' x/protocolpool/types/Length of output: 1583
x/protocolpool/autocli.go (1)
92-99: Approve with suggestions for improvementThe new
UpdateParamscommand is well-structured and consistent with other commands in the file. However, consider the following improvements:
- Enhance the example's readability by using a multi-line string:
Example: fmt.Sprintf(`%s tx protocolpool update-params-proposal ' { "enabled_distribution_denoms": ["stake", "foo"] }'`, version.AppName),
- Provide more details about the expected format of the params in the Short description:
Short: "Submit a proposal to update protocolpool module params. Params should be provided as a JSON object containing all module parameters.",These changes would improve clarity and usability for developers interacting with the CLI.
x/protocolpool/keeper/msg_server.go (1)
228-238: LGTM: New UpdateParams method enhances configurabilityThe addition of the
UpdateParamsmethod is a valuable enhancement, allowing for dynamic updates to the protocol pool parameters. The implementation correctly validates the authority before applying changes.Consider wrapping the error returned from
k.Params.Set()with additional context:if err := k.Params.Set(ctx, msg.Params); err != nil { return nil, fmt.Errorf("failed to set params: %w", err) }This would provide more informative error messages for debugging.
x/protocolpool/keeper/keeper_test.go (3)
82-86: LGTM: Initialization of poolKeeper paramsThe addition of parameter initialization for the poolKeeper is a good practice. It ensures that the keeper starts with known parameters for testing.
Consider extracting the default bond denom into a constant at the package level for easier maintenance and reusability across tests.
148-155: LGTM: Updated Walk function and assertions in TestIterateAndUpdateFundsDistributionThe changes to the Walk function and assertions correctly reflect the update in the data structure used for distribution amounts. The use of
types.DistributionAmountand checking theAmountfield is consistent with the modifications described in the AI summary.Consider using
s.Assert().Equal()instead ofs.Require().Equal()for these assertions, as they don't affect the flow of the test andAssertis generally preferred for non-critical checks in tests.
216-216: LGTM: Updated assertions and Walk function in TestSetToDistributeThe changes to use
sdk.Coinsinstead ofmath.Intand the update to the Walk function to usetypes.DistributionAmountare consistent with the overall modifications to the protocol pool's data structures. These changes correctly reflect the new approach to handling distribution amounts.Consider adding a comment explaining the significance of the
DistributionAmounttype and itsAmountfield, as this change represents a significant shift in how distribution amounts are handled.Also applies to: 219-220, 225-225, 228-229, 236-236
x/protocolpool/keeper/keeper.go (5)
Line range hint
46-74: LGTM: Consistent updates to NewKeeper functionThe changes in the
NewKeeperfunction correctly reflect the modifications made to theKeeperstruct. The removal of thesk types.StakingKeeperparameter is consistent with the decoupling from staking-related functionality.One minor suggestion for improvement:
Consider wrapping the
paniccalls with a custom function that includes the module name, to improve error traceability. For example:func panicModuleAccountNotSet(accountName string) { panic(fmt.Sprintf("%s: %s", types.ModuleName, fmt.Sprintf(errModuleAccountNotSet, accountName))) } // Usage if addr := ak.GetModuleAddress(types.ModuleName); addr == nil { panicModuleAccountNotSet(types.ModuleName) }This approach would make it easier to identify which module caused the panic in a larger application context.
117-137: LGTM: Improved multi-denomination support in withdrawRecipientFundsThe changes to
withdrawRecipientFundsfunction successfully implement support for multiple coin denominations, aligning with the PR objective. The switch fromsdk.Cointosdk.Coinsallows for more flexible fund distribution.However, there's room for improvement in error handling:
Consider using
errors.Jointo combine multiple errors when appropriate, and ensure consistent error wrapping. For example:if err != nil { return nil, errors.Join(err, fmt.Errorf("failed to distribute funds for recipient %s", recipient)) }This approach provides more context and maintains a consistent error handling pattern throughout the codebase.
Line range hint
147-208: Approve with suggestions: Improved multi-denomination support in SetToDistributeThe changes to
SetToDistributefunction successfully implement support for multiple coin denominations and introduce a whitelist mechanism, which aligns with the PR objective and adds flexibility to the system.However, there are a couple of points to consider:
The early return condition on line 160 might be too strict. Consider allowing the function to continue even if the current balance is zero, as this might be a valid state in some scenarios. If early return is necessary, add a comment explaining why.
The comparison between
currentBalanceandlastBalance.Amounton line 174 might not account for all scenarios with multiple denominations. Consider implementing a more robust comparison that handles cases where denominations might be present in one balance but not the other.Example improvement for point 2:
amountToDistribute := sdk.NewCoins() for _, coin := range currentBalance { lastAmount := lastBalance.Amount.AmountOf(coin.Denom) if coin.Amount.GT(lastAmount) { amountToDistribute = amountToDistribute.Add(sdk.NewCoin(coin.Denom, coin.Amount.Sub(lastAmount))) } } if amountToDistribute.IsZero() { return nil }This approach ensures that we're only distributing positive differences for each denomination.
Line range hint
230-285: Approve with concern: Improved multi-denomination support in IterateAndUpdateFundsDistributionThe changes to
IterateAndUpdateFundsDistributionfunction successfully implement support for multiple coin denominations, which aligns with the PR objective. The logic for calculating distributions and remaining pool funds has been updated to handle multiple denominations correctly.However, there's a potential issue to address:
On line 251, the use of
TruncateInt()when calculating the amount to distribute for each denomination could lead to precision loss, especially for small percentages or amounts. This might result in some funds never being distributed due to consistent rounding down.Consider using a more precise approach, such as keeping track of remainders and carrying them forward to the next distribution. For example:
remainders := make(map[string]sdk.Dec) // ... in the distribution loop: for _, denom := range amount.Amount.Denoms() { fullAmount := f.Percentage.MulInt(amount.Amount.AmountOf(denom)).Add(remainders[denom]) intPart := fullAmount.TruncateInt() remainders[denom] = fullAmount.Sub(sdk.NewDecFromInt(intPart)) am := sdk.NewCoin(denom, intPart) // ... rest of the distribution logic }This approach would help ensure that all funds are eventually distributed, minimizing the impact of rounding errors over time.
Line range hint
1-516: Summary: Successful implementation of multi-denomination support with some areas for improvementThe changes in this file successfully implement support for multiple denominations in the protocol pool module, achieving the main objective of the PR. Key improvements include:
- Removal of the staking keeper dependency, reducing coupling.
- Consistent use of
types.DistributionAmountfor flexible handling of multiple denominations.- Updated logic in key functions like
SetToDistributeandIterateAndUpdateFundsDistributionto support multiple coins.- Improved efficiency in module transfers by avoiding zero-amount operations.
Areas for potential improvement:
- Address potential precision loss in distribution calculations, particularly in the
IterateAndUpdateFundsDistributionfunction.- Enhance error handling consistency throughout the module, particularly in the
withdrawRecipientFundsfunction.- Consider revising the early return condition in the
SetToDistributefunction to handle edge cases more robustly.Overall, the changes represent a significant improvement in the module's functionality and align well with the PR objectives. Addressing the identified issues will further enhance the robustness and reliability of the implementation.
x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (1)
46-54: Confirm the necessity of Gogoproto optionsThe
amountfield includes Gogoproto options like(gogoproto.nullable) = falseand(gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins". Ensure that these options are necessary for your use case, as excessive use of custom options can complicate maintenance.api/cosmos/protocolpool/v1/types.pulsar.go (1)
2501-2501: Add a comment for the exported fieldAmountConsider adding a comment for the exported field
Amountto enhance code readability and maintain consistency with other structs.You might add a comment like:
// Amount holds a list of coins representing the periodic distribution amounts.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
⛔ Files ignored due to path filters (4)
api/cosmos/protocolpool/v1/tx_grpc.pb.gois excluded by!**/*.pb.gox/protocolpool/types/genesis.pb.gois excluded by!**/*.pb.gox/protocolpool/types/tx.pb.gois excluded by!**/*.pb.gox/protocolpool/types/types.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
- api/cosmos/protocolpool/v1/genesis.pulsar.go (38 hunks)
- api/cosmos/protocolpool/v1/types.pulsar.go (6 hunks)
- x/protocolpool/autocli.go (1 hunks)
- x/protocolpool/keeper/genesis.go (4 hunks)
- x/protocolpool/keeper/keeper.go (10 hunks)
- x/protocolpool/keeper/keeper_test.go (7 hunks)
- x/protocolpool/keeper/msg_server.go (2 hunks)
- x/protocolpool/keeper/msg_server_test.go (19 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/genesis.proto (1 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto (4 hunks)
- x/protocolpool/proto/cosmos/protocolpool/v1/types.proto (2 hunks)
- x/protocolpool/testutil/expected_keepers_mocks.go (1 hunks)
- x/protocolpool/types/expected_keepers.go (1 hunks)
- x/protocolpool/types/genesis.go (1 hunks)
- x/protocolpool/types/keys.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
api/cosmos/protocolpool/v1/genesis.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.api/cosmos/protocolpool/v1/types.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/autocli.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.x/protocolpool/keeper/keeper.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/keeper_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"x/protocolpool/keeper/msg_server.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/keeper/msg_server_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"x/protocolpool/testutil/expected_keepers_mocks.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/expected_keepers.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/genesis.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/protocolpool/types/keys.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
📓 Learnings (1)
x/protocolpool/keeper/genesis.go (2)
Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2024-10-08T15:31:05.486Z 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.Learnt from: likhita-809 PR: cosmos/cosmos-sdk#18471 File: x/protocolpool/keeper/genesis.go:12-51 Timestamp: 2023-11-22T12:32:39.368Z 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.
🪛 buf
x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto
6-6: import "cosmos/protocolpool/v1/types.proto": file does not exist
(COMPILE)
🔇 Additional comments (63)
x/protocolpool/types/genesis.go (1)
9-9: LGTM: SDK types import addedThe addition of the SDK types import is consistent with the changes in the struct fields and necessary for using
sdk.NewCoins().x/protocolpool/keeper/msg_server.go (2)
160-162: LGTM: Enhanced flexibility for continuous fundsThis change aligns well with the PR objective of allowing any coins in continuous funds. By using
types.DistributionAmount{Amount: sdk.NewCoins()}instead ofmath.ZeroInt(), the implementation now supports multiple coin denominations, enhancing the flexibility of token usage in the protocol pool.
Line range hint
1-262: Summary: Enhancements align with PR objectivesThe changes in this file successfully implement the PR objectives:
- The modification in
CreateContinuousFundallows for more flexible token usage in continuous funds.- The new
UpdateParamsmethod enhances the module's configurability.These changes improve the protocol pool's functionality without introducing any significant issues. The code maintains consistency with the existing style and error handling patterns.
x/protocolpool/keeper/keeper_test.go (5)
38-42: LGTM: Removal of stakingKeeper from KeeperTestSuiteThe removal of the
stakingKeeperfield from theKeeperTestSuitestruct is consistent with the changes described in the AI summary. This change simplifies the test suite by removing unnecessary dependencies.
100-101: LGTM: Updated balance retrieval in mockWithdrawContinuousFundThe change from
GetAllBalancestoGetBalancefor a specific denomination is consistent with the updated functionality. This modification aligns well with the shift towards handling a single denomination in the continuous fund withdrawal process.
109-110: LGTM: Updated balance retrieval in mockStreamFundsThe modification from
GetAllBalancestoGetBalancefor a specific denomination in themockStreamFundsmethod is consistent with the previous changes. This update maintains the coherence of the test suite with the new single-denomination approach.
124-125: LGTM: Updated balance retrieval in TestIterateAndUpdateFundsDistributionThe change from
GetAllBalancestoGetBalancefor a specific denomination in theTestIterateAndUpdateFundsDistributionmethod is consistent with the previous modifications. This change maintains the coherence of the test suite with the new single-denomination approach.
184-185: LGTM: Updated balance retrieval and coin transfer in TestSetToDistributeThe changes from
GetAllBalancestoGetBalancefor a specific denomination and the update to theSendCoinsFromModuleToModulecall are consistent with the previous modifications. These changes correctly reflect the shift to single-denomination handling in the protocol pool.Also applies to: 188-188
x/protocolpool/keeper/keeper.go (3)
Line range hint
24-39: LGTM: Improved Keeper structure and flexibilityThe changes to the
Keeperstruct look good. Removing thestakingKeeperdependency reduces coupling, while the use oftypes.DistributionAmountforRecipientFundDistribution,Distributions, andLastBalanceallows for more flexible handling of multiple coin denominations. This change aligns well with the PR objective of allowing any coins in continuous funds.
277-287: LGTM: Improved efficiency in module transfersThe changes in this section are well-implemented and improve the efficiency of the module. The conditional checks for zero amounts before performing module transfers prevent unnecessary operations, which is a good practice. Additionally, the separation of transfers for distribution amounts and pool funds enhances the clarity and maintainability of the code.
These improvements align well with the overall goal of supporting multiple denominations in the protocol pool, while also optimizing the transfer operations.
307-314: LGTM: Correct implementation of multi-denomination support in recipient fund distributionThe changes in this section successfully implement the handling of multiple denominations in the recipient fund distribution. The use of
types.DistributionAmountand theAddmethod for combining coins ensures that all denominations are properly accounted for when updating the claimable amounts for each recipient.This implementation aligns well with the PR objective of allowing any coins in continuous funds and maintains consistency with the changes made in other parts of the module.
x/protocolpool/types/expected_keepers.go (1)
20-20:⚠️ Potential issueAdding
GetBalancetoBankKeeperinterface may introduce breaking changesAdding a new method to an interface requires all implementations of
BankKeeperto implement this new method. Please ensure that all types implementingBankKeeperhave been updated to includeGetBalanceto prevent build failures.x/protocolpool/proto/cosmos/protocolpool/v1/genesis.proto (1)
27-27: Ensure Backward Compatibility with AddedParamsFieldAdding the
Paramsfield to theGenesisStatemessage enhances configurability. Please ensure that the system handles cases where existing genesis files might not include this new field. Implement default values or update mechanisms to maintain compatibility with older genesis files.x/protocolpool/keeper/genesis.go (3)
16-19: Parameters are set correctly with error handlingThe added code properly sets the parameters from the genesis state and appropriately wraps the error to provide context, aligning with best practices.
63-63: Appropriate update to usesdk.CoinsfortotalToBeDistributedChanging
totalToBeDistributedfrom anInttosdk.Coinscorrectly accommodates multiple coin types, enhancing the flexibility of the distribution logic.Also applies to: 65-65
73-75: Validation logic for total distribution is correctly implementedThe condition now accurately checks if
totalToBeDistributedexceedsdata.LastBalance.Amount, which aligns with the error message and ensures that the distribution does not surpass the available balance.x/protocolpool/testutil/expected_keepers_mocks.go (2)
132-138: MethodGetBalanceadded toMockBankKeeperThe added method correctly mocks the
GetBalancefunction, adhering to the standard pattern for GoMock implementations.
140-145: Recorder methodGetBalanceadded toMockBankKeeperMockRecorderThe recorder method for
GetBalanceis properly implemented, matching the mock method signature and conforming to the expected GoMock structure.x/protocolpool/proto/cosmos/protocolpool/v1/tx.proto (3)
165-165: Update 'withdrawn_allocated_fund' field to support multiple coinsThe
withdrawn_allocated_fundfield inMsgCancelContinuousFundResponsehas been changed to arepeatedCoin, allowing representation of multiple coins.This enhancement increases flexibility by supporting multiple coin types.
179-179: Modify 'amount' field to accept multiple coinsThe
amountfield inMsgWithdrawContinuousFundResponseis updated to arepeatedCoin, enabling it to handle multiple coins.This change aligns with the goal of supporting any tokens in continuous funds.
184-202: Add 'MsgUpdateParams' messages to enable parameter updatesThe new messages
MsgUpdateParamsandMsgUpdateParamsResponseare correctly defined to support governance operations for updating module parameters.This addition enhances the module's governance capabilities, allowing for dynamic parameter updates.
x/protocolpool/keeper/msg_server_test.go (15)
398-398: Appropriate Update towithdrawnAmountTypeThe change of
withdrawnAmountfromsdk.Cointosdk.Coinsreflects the updated representation of funds, allowing handling of multiple coin types.
426-427: Initialization of Recipient Fund DistributionSetting the recipient's fund distribution with an empty
sdk.NewCoins()correctly initializes the distribution amount.
486-487: Correct Initialization of Recipient Fund DistributionThe recipient's fund distribution is properly initialized with an empty amount using
sdk.NewCoins().
488-490: Appropriate Setup for Zero Fund DistributionSetting the distributions to zero and mocking stream funds with zero simulates the scenario where no funds are available for distribution.
508-512: Valid Setup for Fund DistributionInitializing the recipient's fund distribution and setting up the distribution with a positive fund amount prepares the test for a valid withdrawal scenario.
533-537: Consistent Initialization for Fund DistributionConsistently initializing recipient fund distributions and setting up distributions aligns with the updated handling of multiple coin types.
559-561: Proper Initialization for Multiple Continuous FundsThe setup correctly prepares multiple continuous funds with their respective recipients and percentages.
574-576: Initialization of Second Recipient Fund DistributionSetting the second recipient's fund distribution ensures accurate test conditions for multiple fund withdrawals.
589-591: Initialization of Third Recipient Fund DistributionAdding the third recipient's fund distribution completes the setup for testing distribution among multiple recipients.
797-797: Update towithdrawnFundsType is AppropriateChanging
withdrawnFundsfromsdk.Cointosdk.Coinsaccommodates multiple coin types, aligning with the updated funds representation.
832-833: Correct Setup of Recipient Fund DistributionInitializing the recipient's fund distribution with
sdk.NewCoins()ensures accurate starting conditions for the test.
847-848: Setting Up Second Recipient's Fund DistributionProperly setting up the second recipient's fund distribution prepares the test for scenarios involving multiple recipients.
884-885: Correct Initialization for Third Recipient's Fund DistributionSetting up the third recipient's fund distribution completes the test setup for canceling continuous funds among multiple recipients.
1007-1007: Assertion is AppropriateUsing
IsZero()to check thatWithdrawnAllocatedFundis empty ensures accurate verification of the fund withdrawal status.
1042-1053: Added TestTestUpdateParamsEnhances CoverageThe new test function
TestUpdateParamseffectively verifies the updating of pool parameters, ensuring that the functionality works as expected.api/cosmos/protocolpool/v1/genesis.pulsar.go (26)
177-177: Ensure Field Descriptor for 'Params' Field is CorrectThe field descriptor
fd_GenesisState_paramsis added on line 177 and assigned on line 187. Verify that the field name"params"used inmd_GenesisState.Fields().ByName("params")matches the actual field name in theGenesisStatestruct to prevent any issues during reflection.Also applies to: 187-187
267-268: Include New Fields in 'Range' Method IterationIn the
Rangemethod, the new fieldsLastBalanceandParamsare included correctly (lines 267-268 and 279-284). Ensure that the iteration handlesnilvalues properly to prevent potentialnilpointer dereferences.Also applies to: 279-284
305-305: Update 'Has' Method for New FieldsThe
Hasmethod now includes checks forLastBalance(line 305) andParams(line 309). This allows for accurate detection of whether these fields are set.Also applies to: 308-309
331-331: Modify 'Clear' Method to Reset New FieldsIn the
Clearmethod, the new fieldsLastBalance(line 331) andParams(line 335) are set tonil. This ensures that callingClearwill reset these fields as expected.Also applies to: 334-335
405-405: Ensure Proper Assignment in 'Set' MethodThe
Setmethod assigns the new fieldsLastBalance(line 405) andParams(line 411) correctly by casting the provided value to the appropriate type.Also applies to: 410-411
444-448: Initialize New Fields in 'Mutable' MethodIn the
Mutablemethod,LastBalance(lines 445-448) andParams(lines 456-459) are initialized if they arenil. This ensures that mutable references can be safely used without causingnilpointer exceptions.Also applies to: 455-459
480-481: Correct 'NewField' Method Implementation for New FieldsThe
NewFieldmethod correctly creates new instances ofDistributionAmount(line 481) andParams(line 487) for the fieldsLastBalanceandParams, respectively.Also applies to: 485-487
569-570: Include New Fields in 'Size' CalculationIn the
Sizemethod, the size ofLastBalance(lines 569-570) andParams(lines 579-582) is included when they are notnil. This ensures accurate size calculation for serialization.Also applies to: 579-582
612-625: Handle New Fields in 'Marshal' MethodThe
Marshalmethod serializesParams(lines 612-625) andLastBalance(lines 642-652) when they are notnil. Ensure that error handling properly captures any issues during marshaling.Also applies to: 642-652
Line range hint
809-839: Update 'Unmarshal' Method to Deserialize New FieldsThe
Unmarshalmethod now includes logic to deserializeLastBalance(lines 809-839). It correctly initializes the field if necessary and unmarshals the data.
875-909: Add Deserialization for 'Params' in 'Unmarshal' MethodLines 875-909 incorporate deserialization logic for the
Paramsfield. The method ensuresParamsis initialized and properly handles potential errors during unmarshaling.
949-949: Define Field Descriptor for 'Amount' Field in 'Distribution'The field descriptor
fd_Distribution_amountis added on line 949 and assigned on line 956. Confirm that"amount"matches the field name in theDistributionstruct to ensure correct reflection behavior.Also applies to: 956-956
1030-1035: Include 'Amount' Field in 'Range' Method of 'Distribution'In the
Rangemethod offastReflection_Distribution, the newAmountfield is included (lines 1030-1035). This allows iteration over all populated fields, including the new ones.
1053-1054: Update 'Has' Method for 'Distribution' StructThe
Hasmethod now checks for the presence of theAmountfield (line 1054), ensuring it correctly reports whether the field is set.
1073-1074: Modify 'Clear' Method in 'Distribution' StructIn the
Clearmethod, theAmountfield is set tonil(line 1074), ensuring that clearing the message resets this field.
1094-1096: Verify 'Get' Method Implementation in 'Distribution'Lines 1094-1096 correctly retrieve
TimeandAmountfields in theGetmethod offastReflection_Distribution. This resolves the previous issue mentioned in past review comments.
1119-1120: Correct Type Casting in 'Set' MethodThe
Setmethod assigns theAmountfield by casting the message to*DistributionAmount(line 1120). This ensures type safety and correctness.
1147-1150: Initialize 'Amount' Field in 'Mutable' MethodThe
Mutablemethod initializesx.Amountif it isnil(lines 1147-1150). This allows for safe mutable operations on theAmountfield.
1167-1169: Implement 'NewField' Method for 'Amount' FieldIn the
NewFieldmethod, a new instance ofDistributionAmountis created forAmount(lines 1167-1169). This provides a default value that can be assigned to the field.
1243-1246: Include 'Amount' in 'Size' CalculationThe
Sizemethod accounts for theAmountfield when it is notnil(lines 1243-1246). This ensures accurate serialization size.
Line range hint
1276-1288: Serialize 'Amount' Field in 'Marshal' MethodIn the
Marshalmethod, theAmountfield is serialized when present (lines 1276-1288). The method handles potential errors during marshaling.Also applies to: 1290-1302
Line range hint
1353-1389: Deserialize 'Amount' Field in 'Unmarshal' MethodThe
Unmarshalmethod includes logic to deserialize theAmountfield (lines 1353-1389), ensuring it's properly initialized and populated.
1485-1485: Add New Fields to 'GenesisState' StructThe
GenesisStatestruct includes new fieldsLastBalance *DistributionAmount(line 1485) andParams *Params(line 1490). Verify that the JSON tags and field options are correct.Also applies to: 1490-1490
1527-1531: Update Getter Methods for New Fields in 'GenesisState'The methods
GetLastBalance(lines 1527-1531) andGetParams(lines 1541-1547) return the new fields correctly. Ensure these methods handlenilcases appropriately.Also applies to: 1541-1547
1553-1554: Modify 'Distribution' Struct and Accessors for New 'Amount' FieldThe
Distributionstruct now includesAmount *DistributionAmount(line 1554). The getter methodsGetTimeandGetAmountare updated accordingly (lines 1577-1581 and 1584-1586).Also applies to: 1577-1581, 1584-1586
1614-1636: Ensure Protobuf Definitions Reflect ChangesIn the protobuf definitions (lines 1614-1636), the
last_balanceandparamsfields are added with appropriate field numbers and types. Confirm that there are no conflicts with existing field numbers.api/cosmos/protocolpool/v1/types.pulsar.go (1)
2495-2529:DistributionAmountstruct implementation looks goodThe
DistributionAmountstruct is well-defined and adheres to Go conventions.
| @@ -8,6 +8,7 @@ import "cosmos_proto/cosmos.proto"; | |||
| import "cosmos/base/v1beta1/coin.proto"; | |||
| import "google/protobuf/timestamp.proto"; | |||
| import "google/protobuf/duration.proto"; | |||
| import "amino/amino.proto"; | |||
There was a problem hiding this comment.
Avoid importing deprecated Amino encoding library
The import of "amino/amino.proto" introduces a dependency on Amino, which is deprecated in favor of Protobuf encoding in the Cosmos SDK. It's recommended to avoid using Amino in new code to ensure future compatibility and reduce technical debt.
| // DistributionAmount is used to store the coins of periodic distributions. | ||
| message DistributionAmount { | ||
| repeated cosmos.base.v1beta1.Coin amount = 1 [ | ||
| (gogoproto.nullable) = false, | ||
| (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins", | ||
| (amino.dont_omitempty) = true, | ||
| (amino.encoding) = "legacy_coins" | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Reassess the use of Amino options in DistributionAmount
The amount field in the DistributionAmount message uses Amino-specific options such as (amino.dont_omitempty) and (amino.encoding) = "legacy_coins". Since Amino is deprecated, consider removing these options and relying solely on Protobuf serialization to align with current best practices.
| // Params defines the parameters for the protocolpool module. | ||
| message Params { | ||
| // enabled_distribution_denoms lists the denoms that are allowed to be distributed. | ||
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. | ||
| repeated string enabled_distribution_denoms = 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding validation for enabled_distribution_denoms
In the Params message, the enabled_distribution_denoms field lacks explicit validation. To prevent potential misconfigurations, consider adding validation logic to ensure that only allowed denominations are included.
| err = k.Distributions.Walk(ctx, nil, func(key time.Time, value types.DistributionAmount) (stop bool, err error) { | ||
| genState.Distributions = append(genState.Distributions, &types.Distribution{ | ||
| Time: &key, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap error when walking through distributions for enhanced clarity
Wrapping the error from k.Distributions.Walk(ctx, nil, ...) with context can provide better insight during debugging and maintain consistency in error handling.
Apply this diff to wrap the error:
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to walk through distributions: %w", err)
}Committable suggestion was skipped due to low confidence.
| params, err := k.Params.Get(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap error when retrieving parameters for better context
To improve error handling and aid in debugging, consider wrapping the error returned by k.Params.Get(ctx) with additional context.
Apply this diff to wrap the error:
if err != nil {
- return nil, err
+ return nil, fmt.Errorf("failed to get params: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| params, err := k.Params.Get(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| params, err := k.Params.Get(ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get params: %w", err) | |
| } |
| err = suite.poolKeeper.Distributions.Set(suite.ctx, suite.ctx.HeaderInfo().Time, types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100000)))}) | ||
| suite.Require().NoError(err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Parameterizing Coin Denomination
Since the protocol now allows any token for continuous funds, consider making the coin denomination configurable rather than hardcoding sdk.DefaultBondDenom.
Apply this diff to parameterize the coin denomination:
- err = suite.poolKeeper.Distributions.Set(suite.ctx, suite.ctx.HeaderInfo().Time, types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(100000)))})
+ denom := <desired_denomination>
+ err = suite.poolKeeper.Distributions.Set(suite.ctx, suite.ctx.HeaderInfo().Time, types.DistributionAmount{Amount: sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(100000)))})Committable suggestion was skipped due to low confidence.
| @@ -625,10 +625,10 @@ | |||
| if len(tc.recipientAddress) > 1 { | |||
| toClaim, err := suite.poolKeeper.RecipientFundDistribution.Get(suite.ctx, tc.recipientAddress[1]) | |||
| suite.Require().NoError(err) | |||
| suite.Require().Equal(toClaim, math.NewInt(20000)) | |||
| suite.Require().Equal(toClaim.Amount, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(20000)))) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Hardcoding Coin Denomination
To enhance flexibility, consider parameterizing the coin denomination instead of using the hardcoded "stake".
Apply this diff:
- suite.Require().Equal(toClaim.Amount, sdk.NewCoins(sdk.NewCoin("stake", math.NewInt(20000))))
+ denom := <desired_denomination>
+ suite.Require().Equal(toClaim.Amount, sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(20000))))Committable suggestion was skipped due to low confidence.
| @@ -865,7 +865,7 @@ | |||
| suite.Require().Error(err) | |||
| suite.Require().ErrorIs(err, collections.ErrNotFound) | |||
| }, | |||
| withdrawnFunds: sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(20000)), | |||
| withdrawnFunds: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(20000))), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parameterize Coin Denomination in withdrawnFunds
Consider making the coin denomination configurable to reflect the allowance of any token in continuous funds.
Apply this diff:
- withdrawnFunds: sdk.NewCoins(sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(20000))),
+ denom := <desired_denomination>
+ withdrawnFunds: sdk.NewCoins(sdk.NewCoin(denom, math.NewInt(20000))),Committable suggestion was skipped due to low confidence.
| @@ -351,13 +363,16 @@ func (x *fastReflection_GenesisState) Get(descriptor protoreflect.FieldDescripto | |||
| return protoreflect.ValueOfList(listValue) | |||
| case "cosmos.protocolpool.v1.GenesisState.last_balance": | |||
| value := x.LastBalance | |||
| return protoreflect.ValueOfString(value) | |||
| return protoreflect.ValueOfMessage(value.ProtoReflect()) | |||
There was a problem hiding this comment.
Add Nil Checks in 'Get' Method for New Fields
In the Get method, lines 366 and 373-375 handle LastBalance and Params. To prevent potential nil pointer dereferences, consider adding checks to handle cases where these fields might be nil.
Apply this diff to add nil handling:
case "cosmos.protocolpool.v1.GenesisState.last_balance":
+ if x.LastBalance == nil {
+ return protoreflect.Value{}
+ }
value := x.LastBalance
return protoreflect.ValueOfMessage(value.ProtoReflect())
case "cosmos.protocolpool.v1.GenesisState.params":
+ if x.Params == nil {
+ return protoreflect.Value{}
+ }
value := x.Params
return protoreflect.ValueOfMessage(value.ProtoReflect())Also applies to: 373-375
| // enabled_distribution_denoms lists the denoms that are allowed to be distributed. | ||
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. |
There was a problem hiding this comment.
Correct the field comment to follow GoDoc conventions
The comment for the exported field EnabledDistributionDenoms should start with the field name capitalized, as per GoDoc guidelines.
Apply this diff to fix the comment:
- // enabled_distribution_denoms lists the denoms that are allowed to be distributed.
+ // EnabledDistributionDenoms lists the denoms that are allowed to be distributed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // enabled_distribution_denoms lists the denoms that are allowed to be distributed. | |
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. | |
| // EnabledDistributionDenoms lists the denoms that are allowed to be distributed. | |
| // This is to avoid spending time distributing undesired tokens to continuous funds and budgets. |
| x.EnabledDistributionDenoms = *clv.list | ||
| default: | ||
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.protocolpool.v1.Params")) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if fd.IsExtension() { | ||
| panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.protocolpool.v1.Params")) | ||
| } | ||
| panic(fmt.Errorf("message cosmos.protocolpool.v1.Params does not contain field %s", fd.FullName())) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
Very nice work. 🏄
I added some minor comments about variable names but feel free to ignore them.
The default genesis though does not work for custom token denoms. We should provide a way to costomize this for chains. IMHO this needs to be solved before merging the changes.
x/protocolpool/keeper/keeper.go
Outdated
| toDistribute := map[string]math.Int{} | ||
| poolFunds := math.ZeroInt() | ||
| fullAmountToDistribute := math.ZeroInt() | ||
| toDistribute := map[string]sdk.Coins{} |
There was a problem hiding this comment.
naming is hard
personal preference: how about distributeToRecipient ?
There was a problem hiding this comment.
accepting these changes 👌
x/protocolpool/keeper/keeper.go
Outdated
| poolFunds := math.ZeroInt() | ||
| fullAmountToDistribute := math.ZeroInt() | ||
| toDistribute := map[string]sdk.Coins{} | ||
| amountToDistribute := sdk.NewCoins() // amount assigned to distributions |
There was a problem hiding this comment.
personal preference: effectiveDistributionAmounts ?
x/protocolpool/keeper/keeper.go
Outdated
| fullAmountToDistribute := math.ZeroInt() | ||
| toDistribute := map[string]sdk.Coins{} | ||
| amountToDistribute := sdk.NewCoins() // amount assigned to distributions | ||
| allDistributions := sdk.NewCoins() // total amount distributed to the pool, to then calculate the remaining pool funds |
There was a problem hiding this comment.
personal preference: totalDistributionAmounts ?
x/protocolpool/keeper/keeper.go
Outdated
| percentageToDistribute := math.LegacyZeroDec() | ||
| allDistributions = allDistributions.Add(amount.Amount...) |
There was a problem hiding this comment.
personal preference: totalPercentageApplied ?
| return nil, err | ||
| } | ||
|
|
||
| return &types.MsgUpdateParamsResponse{}, nil |
There was a problem hiding this comment.
Adding/removing a denom from the list does not affect the Distributions stored but only future distributions. I assume this the expected behaviour but I wanted to check.
x/protocolpool/types/genesis.go
Outdated
| } | ||
| } | ||
|
|
||
| func DefaultGenesisState() *GenesisState { | ||
| return &GenesisState{ | ||
| ContinuousFund: []*ContinuousFund{}, | ||
| Budget: []*Budget{}, | ||
| Params: &Params{ | ||
| EnabledDistributionDenoms: []string{"stake"}, |
There was a problem hiding this comment.
Blocker: We need a way to make this configurable by chains.
Not sure if sdk.DefaultBondDenom is still used but you can make this a constructor param and pass it from the app module
There was a problem hiding this comment.
They can change this in the genesis file, but I'll change it to sdk.DefaultBondDenom, not sure what I thought about when I put there the string
|
The changes in this PR would be state breaking. The common way is to handle this, is to bump the module version and provide a state migration. See https://github.com/cosmos/cosmos-sdk/blob/main/x/group/migrations/v2/migrate.go for an example |
No need for migration as this module hasn't been published yet |
There was a problem hiding this comment.
would be great to add a test case for a non allowed denom
There was a problem hiding this comment.
Non allowed denoms are just ignored in the case of continuous funds. In budgets there's no limit as to what denom you can use.
For example, we could be distributing ATOMs to continuous funds, but the community pool also contains USDC which can be still used for budgets and not distributed.
There was a problem hiding this comment.
I still modified TestSetToDistribute to have 2 coins, just in case
| // UpdateParams defines a governance operation for updating the x/protocolpool module parameters. | ||
| // The authority is defined in the keeper. | ||
| rpc UpdateParams(MsgUpdateParams) returns (MsgUpdateParamsResponse) { | ||
| option (cosmos_proto.method_added_in) = "cosmos-sdk 0.47"; |
There was a problem hiding this comment.
should be changed to 0.52 or protocolpool v0.2.0
(cherry picked from commit 13c234f) # Conflicts: # api/cosmos/protocolpool/v1/genesis.pulsar.go # api/cosmos/protocolpool/v1/tx.pulsar.go # api/cosmos/protocolpool/v1/tx_grpc.pb.go # api/cosmos/protocolpool/v1/types.pulsar.go # x/protocolpool/keeper/genesis.go # x/protocolpool/testutil/expected_keepers_mocks.go
* main: (31 commits) docs: update links for https security protocol (#22514) build(deps): Bump github.com/bytedance/sonic from 1.12.3 to 1.12.4 in /log (#22513) feat(x/protocolpool)!: allow any coins in continuous funds (#21916) docs: Update protobuf tx signing message format outer link (#22510) test(accounts): fix integration tests (#22418) chore(x): fix some typos in comment (#22508) build(deps): Bump cosmossdk.io/log from 1.4.1 to 1.5.0 (#22487) build(deps): Bump cosmossdk.io/core from 1.0.0-alpha.5 to 1.0.0-alpha.6 (#22502) build(deps): Bump golang.org/x/crypto from 0.28.0 to 0.29.0 (#22480) docs(adr75): server v2 (#21069) fix(server/v2): improve server stop (#22455) chore: prepare core changelog (#22495) refactor(store/v2): simplify genesis flow (#22435) build(deps): Bump google.golang.org/grpc from 1.67.1 to 1.68.0 (#22486) build(deps): Bump golang.org/x/sync from 0.8.0 to 0.9.0 (#22482) feat(x/circuit): Allow msg Reset with empty msgURL (#22459) build(deps): Bump actions/xxx-artifact from v3 to v4 (#22468) feat(stf/branch): simplify merged iterator (#22131) refactor(log): disable coloring in testing logger (#22466) chore(x/tx): update changelog to alpha.2 (#22465) ...
…21916) (#22512) Co-authored-by: Facundo Medica <[email protected]> Co-authored-by: Facundo <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Previously only the bond denom could be used for continuous funds, now we allow any token.
We might want to add a param with a whitelist of tokens to avoid spam.Done withEnabledDistributionDenomsAuthor 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
Summary by CodeRabbit
New Features
DistributionAmountstructure for handling multiple coins.Paramsstructure to manage distribution parameters.UpdateParamsmethod for governance operations.Improvements
LastBalanceandAmountfields to utilize structured types instead of primitive strings.Keeperby removing theStakingKeeperdependency.Bug Fixes
Chores
StakingKeeperinterface and its mock implementations, streamlining the codebase.