fix(x/distribution): fully deprecate distribution community pool #18539
Conversation
WalkthroughWalkthroughThe Cosmos SDK has undergone a significant refactoring, with a focus on the distribution module. Key changes include the deprecation of the Changes
TipsChat with CodeRabbit Bot (
|
julienrbrt
changed the title
|
I have tests and changelog to update and add, but i'd like a quick agreement on the concept of this PR before I continue. |
| ]; | ||
|
|
||
| repeated cosmos.base.v1beta1.DecCoin decimal_pool = 2 |
There was a problem hiding this comment.
This is the same as the community pool in terms of type, but renaming so that there is no confusion
There was a problem hiding this comment.
what is decimal pool suppose to signify?
There was a problem hiding this comment.
That it gets the decimal coins, the scrums. This concept does not need to be known outside the module, but keeping the community pool as said above would have been confusing.
| @@ -9,23 +9,24 @@ import ( | |||
| ) | |||
|
|
|||
| // MigrateFunds migrates the distribution module funds to pool module | |||
| func MigrateFunds(ctx sdk.Context, bankKeeper types.BankKeeper, feePool types.FeePool, macc, poolMacc sdk.ModuleAccountI) error { | |||
| poolBal := sdk.NormalizeCoins(feePool.CommunityPool) | |||
There was a problem hiding this comment.
Here the dust would stay in the community pool and as the previous migration was resetting it it could not match the supply and after the migration it would keep accruing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Configuration used: CodeRabbit UI
Files ignored due to filter (9)
- api/cosmos/nft/v1beta1/query_grpc.pb.go
- testutil/testdata/query.pb.go
- testutil/testdata/testdata.pb.go
- testutil/testdata/testpb/query_grpc.pb.go
- testutil/testdata/testpb/tx_grpc.pb.go
- testutil/testdata/tx.pb.go
- testutil/testdata/unknonwnproto.pb.go
- x/distribution/types/distribution.pb.go
- x/nft/query.pb.go
Files selected for processing (34)
- CHANGELOG.md (5 hunks)
- api/cosmos/distribution/v1beta1/distribution.pulsar.go (15 hunks)
- api/cosmos/nft/v1beta1/query.pulsar.go (10 hunks)
- proto/cosmos/distribution/v1beta1/distribution.proto (1 hunks)
- simapp/export.go (1 hunks)
- tests/integration/distribution/keeper/msg_server_test.go (1 hunks)
- testutil/testdata/testpb/query.pulsar.go (1 hunks)
- testutil/testdata/testpb/testdata.pulsar.go (1 hunks)
- testutil/testdata/testpb/tx.pulsar.go (1 hunks)
- testutil/testdata/testpb/unknonwnproto.pulsar.go (1 hunks)
- x/distribution/CHANGELOG.md (1 hunks)
- x/distribution/README.md (15 hunks)
- x/distribution/abci.go (1 hunks)
- x/distribution/keeper/allocation.go (4 hunks)
- x/distribution/keeper/allocation_test.go (5 hunks)
- x/distribution/keeper/delegation.go (3 hunks)
- x/distribution/keeper/genesis.go (3 hunks)
- x/distribution/keeper/hooks.go (2 hunks)
- x/distribution/keeper/invariants.go (1 hunks)
- x/distribution/keeper/keeper.go (1 hunks)
- x/distribution/keeper/migrations.go (2 hunks)
- x/distribution/keeper/msg_server.go (1 hunks)
- x/distribution/keeper/msg_server_test.go (1 hunks)
- x/distribution/keeper/validator.go (2 hunks)
- x/distribution/migrations/v4/migrate_funds.go (2 hunks)
- x/distribution/migrations/v4/migrate_funds_test.go (3 hunks)
- x/distribution/module.go (2 hunks)
- x/distribution/simulation/decoder_test.go (1 hunks)
- x/distribution/testutil/expected_keepers_mocks.go (1 hunks)
- x/distribution/types/expected_keepers.go (1 hunks)
- x/distribution/types/fee_pool.go (1 hunks)
- x/distribution/types/fee_pool_test.go (1 hunks)
- x/protocolpool/keeper/keeper.go (2 hunks)
- x/protocolpool/keeper/msg_server.go (1 hunks)
Files not summarized due to errors (1)
- api/cosmos/distribution/v1beta1/distribution.pulsar.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (7)
- api/cosmos/nft/v1beta1/query.pulsar.go
- testutil/testdata/testpb/query.pulsar.go
- testutil/testdata/testpb/testdata.pulsar.go
- x/distribution/README.md
- x/distribution/keeper/invariants.go
- x/distribution/simulation/decoder_test.go
- x/distribution/types/expected_keepers.go
Additional comments: 57
simapp/export.go (1)
- 126-141:
The changes correctly reflect the transition from using thecommunity_poolto thedecimal_pool. Ensure that all other parts of the codebase that interact with the fee pool are updated accordingly.x/protocolpool/keeper/msg_server.go (1)
- 93-102:
The changes to theCommunityPoolSpendfunction reflect the updated logic and simplified logging. Ensure that the newDistributeFromCommunityPoolmethod is implemented and tested.x/distribution/keeper/msg_server_test.go (1)
- 260-262: The test
TestMsgCommunityPoolSpendhas been updated to expect theDistributeFromCommunityPoolmethod instead of the deprecatedDistributeFromFeePool. Ensure that theDistributeFromCommunityPoolmethod is implemented with the same signature and that all tests are updated accordingly to reflect the new behavior.x/distribution/testutil/expected_keepers_mocks.go (1)
- 249-264: The renaming of
DistributeFromFeePooltoDistributeFromCommunityPoolin theMockPoolKeeperstruct and its recorder is consistent with the changes described in the pull request summary. This change reflects the deprecation of the community fee pool and the transition to using adecimal pool. Ensure that all references to the old method name are updated across the codebase to prevent any integration issues.x/distribution/module.go (2)
31-31: The consensus version has been decreased from 5 to 4. This is a significant change that could potentially cause issues with state migrations and upgrades. Ensure that this change is intentional and that all necessary migration logic has been implemented to handle the version downgrade.
143-145: The
MigrateFundsToPoolmigration function has been removed. Verify that this migration is no longer needed and that any funds previously managed by the community pool are correctly handled after this change.x/distribution/keeper/msg_server.go (1)
- 158-164: The method
DistributeFromCommunityPoolhas been renamed toDistributeFromCommunityPool, reflecting the shift from the deprecated community pool to the new decimal pool system. Ensure that the implementation ofDistributeFromCommunityPoolaligns with the new logic for handling the decimal pool and that all references to the old community pool have been updated accordingly.x/distribution/CHANGELOG.md (1)
- 1-64: - The changelog is well-structured and follows the guiding principles outlined in the comments. It provides a clear and detailed summary of the breaking changes, new features, and other significant alterations to the Cosmos SDK's distribution module.
- Ensure that all links to pull requests are correct and that the referenced changes match the descriptions provided.
- Verify that the migration of community pool funds to
x/protocolpooland the introduction ofFeePool.DecimalPoolare well-documented and communicated to developers, as these are significant changes.- The changelog entries should be reviewed for accuracy and completeness by the team to ensure that all relevant changes are included and that there are no omissions or errors.
x/protocolpool/keeper/keeper.go (1)
- 75-82: The function
DistributeFromCommunityPoolhas been correctly renamed and the comments have been updated to reflect the change from "feepool" to "community pool". This aligns with the summary of changes indicating the deprecation of the community fee pool and the transition to using adecimal pool.x/distribution/types/fee_pool.go (2)
6-15:
The functionInitialFeePoolhas been correctly updated to initialize the newDecimalPoolfield.17-27:
TheValidateGenesismethod has been updated to validate the newDecimalPoolfield. Ensure that the TODO comment is tracked in the project's issue tracker for future implementation.x/distribution/keeper/validator.go (2)
61-67: The code correctly fetches the
feePooland checks for errors, which is good practice for robust error handling. However, it is important to ensure that theFeePool.Getmethod is designed to handle the newdecimal_poolfield correctly.72-78: The logic for adding rewards to the
decimal_pooland updating theoutstanding.Rewardsis sound. However, it is crucial to ensure that theFeePool.SetandValidatorOutstandingRewards.Setmethods are atomic to prevent data races or inconsistencies, especially since this code may be part of a blockchain state transition.testutil/testdata/testpb/unknonwnproto.pulsar.go (1)
- 5-18: - The import of
v1beta1 "cosmossdk.io/api/cosmos/tx/v1beta1"is added, which is likely needed for the changes in the distribution module. Ensure that this import is used in the code.
- The removal of
runtime "github.com/cosmos/cosmos-proto/runtime"and_ "github.com/cosmos/gogoproto/gogoproto"imports suggests that these packages are no longer used. Verify that removing these imports does not affect other parts of the code that may still require them.- The addition of imports for
io,math,reflect,sort, andsyncindicates new functionality that requires these packages. Confirm that these imports are indeed used in the code.- The reordering of imports is a good practice for readability and maintainability.
testutil/testdata/testpb/tx.pulsar.go (1)
- 4-15: - Removed imports for
io,reflect, andsyncare not shown in the hunk, ensure they are indeed removed as mentioned in the summary.
- Added import for
runtimeis correctly shown.- Specific imports from
cosmossdk.ioandcosmospackages are not shown in the hunk, ensure they are indeed removed as mentioned in the summary.- The import block is reordered correctly according to the Go conventions (standard library -> third party -> own packages).
x/distribution/keeper/genesis.go (3)
14-19: The
InitGenesisfunction has been updated to use the newFeePoolstructure with theDecimalPoolfield. Ensure that all other parts of the codebase that interact with theFeePoolare updated accordingly to prevent any runtime errors due to the changed structure.46-51: The error handling for setting the
PreviousProposeris consistent with the rest of the function. However, ensure that thePreviousProposerfield is still relevant after the changes to the distribution module and that it is being used correctly elsewhere in the codebase.130-133: The addition of
data.FeePool.DecimalPooltomoduleHoldingsreplaces the previousCommunityPool. This change aligns with the pull request's goal to deprecate the community pool. Verify that theDecimalPoolis correctly initialized and that the transition from the community pool to the decimal pool is handled in all relevant parts of the codebase.x/distribution/migrations/v4/migrate_funds_test.go (3)
1-4: The package name and import paths have been correctly updated to reflect the new module version.
16-22: The import paths have been updated to use the new module version
v4. Ensure that all references to the old module version have been updated across the entire codebase.100-105: The test has been updated to capture the return value of
MigrateFundsand check for errors, which is a good practice for error handling. However, ensure that theMigrateFundsfunction has been properly updated to return a value that can be checked for errors.x/distribution/keeper/allocation_test.go (5)
151-153: The test checks if the
DecimalPoolis zero, which aligns with the new changes to the distribution module. However, ensure that theFeePoolstruct no longer contains thecommunity_poolfield and that all references to it have been removed or replaced withdecimal_poolthroughout the codebase.168-171: The test simulates sending coins from the
fee_collectormodule to thedistributionmodule and then to theprotocolpoolmodule. Ensure that theSendCoinsFromModuleToModulefunction is properly handling errors and that the amounts being sent are correctly calculated and validated.190-191: The test asserts the expected outstanding rewards for the second validator. Given the significant changes to the distribution process, ensure that the calculations for outstanding rewards are still accurate and that the new
decimal poolmechanism is correctly factored into these calculations.193-195: The test checks if the
DecimalPoolis zero after token allocation. This is consistent with the new changes, but ensure that theFeePoolstruct is correctly updated in the codebase and that theDecimalPoolis being managed as expected.309-312: The test simulates sending a large number of coins and checks if some amount is sent to the
protocolpool. Ensure that theSendCoinsFromModuleToModulefunction is robust against large transactions and that the logic for determining the amount sent to theprotocolpoolis correct and secure.x/distribution/keeper/allocation.go (5)
3-14: The import of the
syncpackage (line 6) and theprotocolpooltypespackage (line 12) are new additions to support the concurrent processing and handling of the newdecimal_pool. Ensure that these packages are used appropriately and that there are no data races introduced by the new concurrent logic.26-42: The logic to handle the case when
totalPreviousPoweris zero (lines 38-42) seems to be adding thefeesCollectedto theDecimalPoolwithout distributing them. This is a significant change in behavior and should be verified for correctness and consistency with the intended design of the newdecimal_poolsystem.51-98: The use of goroutines and channels (lines 54-88) for concurrent processing of bonded votes is a new addition. Ensure that the concurrent logic is correctly implemented and that there are no data races or deadlocks. The use of
sync.WaitGroupand error handling via channels is a good practice, but it's crucial to ensure that all goroutines complete and that errors are properly propagated.100-108: The logic to send the remaining rewards to the community pool and set the remainder in the fee pool (lines 100-108) is new. It's important to verify that the
SendCoinsFromModuleToModulefunction is correctly implemented and that the remainder is correctly added to theDecimalPool. Also, ensure that theFeePoolis correctly updated with the new remainder.176-194: The new function
SendDecimalPoolToCommunityPool(lines 176-194) is responsible for transferring theDecimalPoolto the community pool. Verify that the logic correctly handles the transfer and that any remainder is correctly left in theDecimalPool. Also, ensure that there are no issues with theSendCoinsFromModuleToModulefunction call and that theFeePoolis correctly updated.x/distribution/migrations/v4/migrate_funds.go (3)
1-4:
The package name change from "funds" to "v4" is appropriate for versioning.11-32:
The changes toMigrateFundsfunction correctly handle the migration of funds from the community pool to the decimal pool, including error handling and returning the newtypes.FeePoolstructure.13-13:
The implementation now correctly handles the separation of the pool balance and the remainder usingTruncateDecimal, which should address the issue of dust staying in the community pool as mentioned in the previous review.proto/cosmos/distribution/v1beta1/distribution.proto (1)
- 117-128: - The
FeePoolmessage has been correctly updated to deprecate thecommunity_poolfield and introduce thedecimal_poolfield. This aligns with the summary of changes indicating the transition from the community pool to the decimal pool.
- Ensure that all references to the
community_poolin the codebase are updated to interact with thedecimal_poolinstead.- Verify that the
decimal_poolis being used correctly in the logic of the distribution module, especially in theBeginBlockerfunction and theAllocateTokensfunction as mentioned in the summary.- The use of
repeated cosmos.base.v1beta1.DecCoinfor thedecimal_poolfield is consistent with the other DecCoin fields in the proto file, which is good for consistency.x/distribution/keeper/migrations.go (2)
1-7:
The change summary indicates that import statements were removed, but they are not visible in the provided hunk. Please ensure that the removal of imports does not affect other parts of the code that may rely on them.33-41:
The functionMigrate3to4has been updated to include a call tomigrateFunds. Ensure that this new function is thoroughly tested to prevent any issues during the migration process.CHANGELOG.md (5)
80-85:
The changelog entries provide clear information about the removal of certain functions due to AutoCLI migration and the removal of the Params module.127-132:
The changelog entries provide clear information about the use of collections forUnbondingTypeandHistoricalInfo, and the removal of associated functions.136-142:
The changelog entries provide clear information about the migration to collections and the removal of certain functions and types.148-153:
The changelog entries provide clear information about the changes to context creation,BurnCoinsfunction, and the removal of Amino-related functions.177-178:
The changelog entry provides clear information about the change in how the upgrade module handles the app version.api/cosmos/distribution/v1beta1/distribution.pulsar.go (15)
3706-3767: The code correctly defines a list implementation for the
decimal_poolfield. However, ensure that thecommunity_poolfield is no longer being used elsewhere in the codebase since it has been deprecated.3841-3846: The check for
len(x.DecimalPool) != 0is correct. It ensures that thedecimal_poolfield is only processed if it contains elements.3864-3865: The switch case correctly checks for the presence of elements in the
decimal_pool. This is consistent with the deprecation of thecommunity_pool.3884-3885: Setting
x.DecimalPooltonilis a standard way to clear the slice. Ensure that this is the intended behavior and that there are no side effects elsewhere in the system.3908-3913: The code correctly handles the case where
x.DecimalPoolis not empty and provides a list value for it. This is part of the protobuf message reflection implementation.3938-3941: The assignment
x.DecimalPool = *clv.listis correct, assuming thatclv.listis a pointer to a slice ofDecCoin. This is part of the protobuf message reflection implementation.3968-3973: The initialization of
x.DecimalPoolto an empty slice if it isnilis a good practice to avoid nil pointer dereferences.3990-3992: Creating a new list with an empty slice for
decimal_poolis correct and aligns with the protobuf message reflection implementation.4068-4073: The code correctly calculates the size needed for encoding the
decimal_poolfield. This is part of the protobuf message serialization process.4103-4117: The loop correctly marshals each
DecCoininx.DecimalPoolin reverse order. Ensure that the error handling is consistent with the rest of the codebase.4218-4251: The unmarshalling logic for the
decimal_poolfield is correct. It checks for the correct wire type and handles potential errors such as integer overflow and unexpected EOF.7066-7074: The
FeePoolstruct has been updated to deprecate thecommunity_poolfield and introduce thedecimal_poolfield. Ensure that all references tocommunity_poolin the codebase have been updated or removed.7097-7109: The getter methods for
community_poolanddecimal_poolare correctly marked as deprecated forcommunity_pooland implemented fordecimal_pool. Ensure that the deprecation ofcommunity_poolis clearly communicated to developers and users.7456-7548: The protobuf file descriptor changes reflect the deprecation of
community_pooland the introduction ofdecimal_pool. Ensure that the corresponding documentation and changelogs are updated to reflect these changes.7587-7594: The type references for
decimal_poolare correct and align with the protobuf definitions. Ensure that all related protobuf message types are correctly updated to reflect the use ofdecimal_pool.
|
|
||
| fp2 := types.FeePool{CommunityPool: sdk.DecCoins{{Denom: "stake", Amount: math.LegacyNewDec(-1)}}} | ||
| require.NotNil(t, fp2.ValidateGenesis()) | ||
|
|
||
| fp3 := types.FeePool{DecimalPool: sdk.DecCoins{{Denom: "stake", Amount: math.LegacyNewDec(-1)}}} | ||
| require.NotNil(t, fp3.ValidateGenesis()) | ||
| } |
There was a problem hiding this comment.
The new test case for fp3 is checking for a negative amount in the DecimalPool, which is a valid test for ensuring that the ValidateGenesis method correctly identifies invalid genesis states. However, it would be beneficial to also include a test case where the DecimalPool has a valid positive amount to ensure that the ValidateGenesis method correctly accepts a valid genesis state.
| authKeeper types.AccountKeeper | ||
| bankKeeper types.BankKeeper | ||
| stakingKeeper types.StakingKeeper | ||
| poolKeeper types.PoolKeeper // TODO: Needs to be removed in v0.53 | ||
| poolKeeper types.PoolKeeper | ||
|
|
||
| // the address capable of executing a MsgUpdateParams message. Typically, this | ||
| // should be the x/gov module account. | ||
| authority string | ||
|
|
||
| Schema collections.Schema | ||
| Params collections.Item[types.Params] | ||
| Schema collections.Schema | ||
| Params collections.Item[types.Params] | ||
| // FeePool stores decimal tokens that cannot be yet distributed. | ||
| // In the past it held the community pool, but it has been replaced by x/protocolpool. | ||
| FeePool collections.Item[types.FeePool] |
There was a problem hiding this comment.
- The
poolKeeperfield is still present in theKeeperstruct, but the TODO comment indicating it needs to be removed has been deleted. If thepoolKeeperis no longer needed due to the deprecation of the community pool, it should be removed from the struct to avoid confusion and potential misuse. - The
FeePoolfield has been added to theKeeperstruct to store decimal tokens. Ensure that all interactions with theFeePoolare secure and correctly implemented throughout the module. - The comment on line 37 provides context but could be more descriptive about the transition from the community pool to the
decimal_pooland its implications for the module's functionality.
| // split into integral & remainder | ||
| coins, remainder := commission.TruncateDecimal() | ||
|
|
||
| // remainder to community pool | ||
| // remainder to decimal pool | ||
| feePool, err := h.k.FeePool.Get(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| feePool.CommunityPool = feePool.CommunityPool.Add(remainder...) | ||
| feePool.DecimalPool = feePool.DecimalPool.Add(remainder...) | ||
| err = h.k.FeePool.Set(ctx, feePool) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
- The code retrieves the
FeePooland adds theremainderto theDecimalPool. Ensure that theFeePoolstruct has been updated to include theDecimalPoolfield and that all references to theCommunityPoolhave been properly replaced withDecimalPoolthroughout the codebase. - Also, consider handling the case where
collections.ErrNotFoundis returned when getting theFeePool. This would ensure that theDecimalPoolis initialized properly even if theFeePooldoesn't exist yet.
| } | ||
| } | ||
|
|
||
| // Add outstanding to community pool | ||
| // Add outstanding to decimal pool | ||
| // The validator is removed only after it has no more delegations. | ||
| // This operation sends only the remaining dust to the community pool. | ||
| // This operation sends only the remaining dust to the decimal pool. | ||
| feePool, err := h.k.FeePool.Get(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| feePool.CommunityPool = feePool.CommunityPool.Add(outstanding...) | ||
| feePool.DecimalPool = feePool.DecimalPool.Add(outstanding...) | ||
| err = h.k.FeePool.Set(ctx, feePool) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
- Similar to the previous hunk, ensure that the
FeePoolstruct includes theDecimalPoolfield and that the transition fromCommunityPooltoDecimalPoolis consistent across the entire codebase. - Again, handle the potential
collections.ErrNotFounderror when getting theFeePoolto ensure proper initialization of theDecimalPool.
| } | ||
| } | ||
|
|
||
| // update the outstanding rewards and the community pool only if the | ||
| // transaction was successful | ||
| err = k.ValidatorOutstandingRewards.Set(ctx, sdk.ValAddress(valAddr), types.ValidatorOutstandingRewards{Rewards: outstanding.Sub(rewards)}) | ||
| if err != nil { | ||
| // update the outstanding rewards and the decimal pool only if the transaction was successful | ||
| if err := k.ValidatorOutstandingRewards.Set(ctx, sdk.ValAddress(valAddr), types.ValidatorOutstandingRewards{Rewards: outstanding.Sub(rewards)}); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [248-280]
- The renaming of "community pool" to "decimal pool" in comments and code is consistent with the summary of changes. However, there is a potential issue with the error handling in the
withdrawDelegationRewardsfunction. The error fromk.FeePool.Set(ctx, feePool)is not being checked immediately after the call, which could lead to a situation where the fee pool is not updated correctly but the function continues execution. This should be addressed to ensure that the function exits early in case of an error when setting the fee pool. - Additionally, the logic to transfer the remainder to the decimal pool is correct, but it's important to ensure that the
FeePoolstruct and its methods are properly updated to handle the newDecimalPoolfield as described in the summary of changes. - The use of goroutines and channels for concurrent processing in the
AllocateTokensfunction is not shown in this hunk, but it's crucial to ensure that there are no data races when accessing shared resources like theFeePool.
feePool.DecimalPool = feePool.DecimalPool.Add(remainder...)
+ if err = k.FeePool.Set(ctx, feePool); err != nil {
+ return nil, err
+ }| // check current balance is greater than initial balance | ||
| curBalance := f.bankKeeper.GetAllBalances(f.sdkCtx, sdk.AccAddress(f.valAddr)) | ||
| assert.Assert(t, initBalance.IsAllLTE(curBalance)) |
There was a problem hiding this comment.
The test case "valid msg" in TestMsgWithdrawDelegatorReward has removed the assertions related to checking rewards and outstanding rewards due to the deprecation of the community pool. However, the test still checks if the current balance is greater than the initial balance, which may not be a sufficient condition to ensure that rewards were correctly withdrawn and distributed. Consider adding more robust assertions to validate the new behavior with the decimal pool.
x/distribution/abci.go
Outdated
| if err := k.AllocateTokens(ctx, previousTotalPower, ctx.CometInfo().LastCommit.Votes); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // every 1000 blocks send whole coins from decimal pool to community pool | ||
| if ctx.BlockHeight()%1000 == 0 { | ||
| if err := k.SendDecimalPoolToCommunityPool(ctx); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
- The new logic to transfer coins from the
decimal poolto thecommunity poolevery 1000 blocks is correctly implemented. However, ensure that this change is well-documented and that any potential side effects are considered, especially regarding the timing and impact on the state. - Additionally, consider the implications of using a hardcoded value like
1000for the block interval. It might be beneficial to make this configurable to allow for more flexibility in different network conditions or governance decisions. - Verify that the
SendDecimalPoolToCommunityPoolfunction handles all edge cases, such as when thedecimal poolis empty or when the transfer fails due to network issues. - Ensure that the error handling is robust and that any errors are logged or monitored appropriately.
- Given the use of goroutines and channels in the
AllocateTokensfunction, as mentioned in the summary, verify that there are no data races or concurrency issues when accessing shared resources, especially when theSendDecimalPoolToCommunityPoolfunction is called concurrently.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (3)
- x/distribution/keeper/allocation.go (4 hunks)
- x/distribution/keeper/migrations.go (2 hunks)
- x/protocolpool/keeper/keeper.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/protocolpool/keeper/keeper.go
Additional comments: 7
x/distribution/keeper/migrations.go (3)
1-7:
The removal of unused imports is a good practice for code cleanliness and readability.33-41:
The change in the function name from "MigrateFundsToPool" to "migrateFunds" and its usage in "Migrate3to4" is consistent and follows the Go naming convention for unexported functions.43-59:
The migration logic inmigrateFundsappears to correctly handle the transfer of funds from the distribution module to the protocol pool module. Ensure that theFeePool.GetandFeePool.Setmethods are correctly implemented to handle the newfeePoolstructure.x/distribution/keeper/allocation.go (4)
3-14: The new import
protocolpooltypesis added, ensure that it is used in the code and there are no unused imports.54-97: The use of goroutines and channels for concurrent processing is introduced. Ensure that the goroutines are properly synchronized using
WaitGroupand that the error handling via channels is correctly implemented to avoid data races or deadlocks.100-107: The logic for sending the remaining rewards to the community pool and setting the remainder in the fee pool is implemented. Verify that the
TruncateDecimalmethod correctly separates the integer and decimal parts and that the error handling is correct when sending coins and setting the fee pool.175-193: The new function
SendDecimalPoolToCommunityPoolis added to handle the transfer of the decimal pool to the community pool. Ensure that the logic correctly handles the case when the decimal pool is zero and that the remainder is correctly set in the fee pool after the transfer.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tests/integration/distribution/keeper/msg_server_test.go (3 hunks)
Files skipped from review due to trivial changes (1)
- tests/integration/distribution/keeper/msg_server_test.go
There was a problem hiding this comment.
Thank you for this change @julienrbrt! Some comments:
- Let's make the commit subject more descriptive of what it is solving so:
fix(x/distribution): deprecate community pool in favour of decimalPool - Please use a goroutine to waiting for the concurrent processing and error generation and then also use a cancellable context which will ensure that as soon as the first non-nil error is encountered, that we cease all work as intended, then avoid the need to process all the balances before failing
julienrbrt
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (5 hunks)
Additional comments: 5
CHANGELOG.md (5)
81-86:
The changelog entries are clear and provide direct references to the associated pull requests for the removed methods and modules due to AutoCLI migration and Params module removal.128-133:
The changelog entries are clear and provide direct references to the associated pull requests for the use of collections and the removal of methods fromtypesandKeeper.137-143:
The changelog entries are clear and provide direct references to the associated pull requests for the migration to collections and the removal of methods and types.149-154:
The changelog entries are clear and provide direct references to the associated pull requests for the changes in context creation,BurnCoinsfunction, and deprecation of Amino-related functions.175-183:
The changelog entries are clear and provide direct references to the associated pull requests for the changes in command usage and the replacement ofHistoricalInfowithHistoricalRecord.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/distribution/go.mod
Files selected for processing (1)
- x/distribution/keeper/allocation.go (4 hunks)
Additional comments: 5
x/distribution/keeper/allocation.go (5)
13-13: The addition of
golang.org/x/sync/errgroupis for handling parallel processing, which is a new pattern in this codebase. Ensure that this pattern is safe and consistent with the rest of the codebase.55-100: The use of
errgroup.Groupfor parallel processing is a significant change. Confirm that this concurrent processing does not introduce any non-deterministic behavior that could affect consensus.166-184: The new
SendDecimalPoolToCommunityPoolfunction manages the transfer of funds from thedecimal_poolto the community pool. Ensure that this function is called at the appropriate times to maintain the intended behavior of the fee distribution.162-184: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [103-164]
The
AllocateTokensToValidatorfunction appears unchanged in the provided hunk. Ensure that any changes to this function are consistent with the new fee distribution logic.
- 26-42: The logic for adding fees to the
DecimalPoolwhentotalPreviousPoweris zero has been clarified by the user in a previous review. Ensure that this logic is still correctly implemented after the changes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/distribution/keeper/allocation.go (4 hunks)
Additional comments: 3
x/distribution/keeper/allocation.go (3)
4-14: The addition of "golang.org/x/sync/errgroup" and "protocolpooltypes" imports aligns with the new concurrent processing logic and the interaction with the protocol pool module.
53-99: The use of errgroup and channels for concurrent processing of bonded votes and handling remaining rewards appears to be implemented correctly. Ensure that the channel is closed only after all goroutines have finished to avoid deadlocks or panics due to sending on a closed channel.
167-184: The new "SendDecimalPoolToCommunityPool" function correctly handles sending the decimal pool to the community pool and managing any remainder. The early return if the DecimalPool is zero is a good practice to avoid unnecessary processing.
| @@ -129,14 +128,10 @@ count is decremented. If the reference count hits zero, the historical record is | |||
|
|
|||
| ### FeePool | |||
|
|
|||
| All globally tracked parameters for distribution are stored within | |||
| `FeePool`. Rewards are collected and added to the reward pool and | |||
There was a problem hiding this comment.
is this not the same as before?
There was a problem hiding this comment.
No, it is stored in the distribution module, what makes it in the fee pool is only the too small coins parts that cannot be sent to the community pool
| ]; | ||
|
|
||
| repeated cosmos.base.v1beta1.DecCoin decimal_pool = 2 |
There was a problem hiding this comment.
what is decimal pool suppose to signify?
x/distribution/keeper/allocation.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| g.Go(func() error { |
There was a problem hiding this comment.
could we have some benchmarks on this?
There was a problem hiding this comment.
could we get a short fuzz test for this function, if possible?
There was a problem hiding this comment.
I agree it would be good to benchmark single threaded vs concurrent. the results might be surprising. iavl v0 & v1 is a single threaded reader, so if this func is i/o bound, there may not be too much wall clock improvement, in fact the coordination overhead could make things slower. iavl v2 scales well with concurrent reads, not sure that's relevant here though.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- CHANGELOG.md (5 hunks)
- x/distribution/README.md (14 hunks)
- x/distribution/keeper/abci.go (3 hunks)
- x/distribution/keeper/allocation.go (4 hunks)
- x/distribution/keeper/allocation_test.go (5 hunks)
- x/distribution/keeper/migrations.go (2 hunks)
- x/distribution/module.go (3 hunks)
- x/distribution/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/distribution/README.md
- x/distribution/types/keys.go
Additional comments: 18
CHANGELOG.md (5)
81-86: The changelog entries clearly communicate the removals and migrations due to AutoCLI migration and the deprecation of the params module. Ensure that dependent modules and applications are updated accordingly.
128-133: The migration to collections and the removal of functions are well-documented. It's important to verify that all references to these removed functions are updated in the codebase to prevent any broken functionality.
137-143: The changelog provides a comprehensive list of changes to the staking, authz, slashing modules, and types. It also includes significant changes to client commands. Developers should ensure that their code is compatible with these changes.
149-154: The changelog notes the deprecation of certain methods and the removal of Amino-related functions. This could impact legacy code, so it's crucial to check for any remaining usages and update them.
173-183: The changelog entry for the server and x/auth/vesting modules indicates command changes and clarifies the retraction of a previous version. It's important to note the state machine breaking changes for the upgrade and staking modules.
x/distribution/keeper/abci.go (2)
1-8: The import statement
cosmossdk.io/x/distribution/typesmight need to be updated to reflect the new package structure if the types have also been moved or renamed.31-35: Verify that the
sendDecimalPoolToCommunityPoolfunction includes logic to check if the decimal pool has sufficient funds before attempting to transfer to the community pool to prevent potential errors.x/distribution/keeper/allocation.go (2)
4-11: The addition of the
errgrouppackage and the use of goroutines and channels for concurrent processing in theAllocateTokensfunction are significant changes. Ensure that all concurrency concerns have been addressed and that the state machine remains deterministic with these changes.166-184: The new
sendDecimalPoolToCommunityPoolfunction correctly checks if the decimal pool is zero before proceeding with sending funds to the community pool. This is a good practice to avoid unnecessary transactions.x/distribution/keeper/allocation_test.go (5)
151-153: The test correctly asserts that the
DecimalPoolis zero after the deprecation of the community pool. This aligns with the changes described in the summary.168-171: The test expectations for
bankKeepercalls correctly reflect the new logic of sending coins to the protocol pool module, which is in line with the deprecation of the community pool and the introduction of the decimal pool.190-191: The test assertions for the validator's outstanding rewards are consistent with the expected values given the new allocation logic.
292-294: The test correctly asserts that the
DecimalPoolis zero, which is consistent with the changes to the community pool and the introduction of the decimal pool.309-312: The test expectations for
bankKeepercalls correctly reflect the new logic of sending coins to the protocol pool module, which is in line with the deprecation of the community pool and the introduction of the decimal pool.x/distribution/keeper/migrations.go (1)
- 33-59: The changes in the migration function
Migrate3to4and the helper functionmigrateFundscorrectly reflect the updates described in the summary. The renaming of the function and the update to the call tov4.MigrateFundsare consistent with the intended changes. The comment on line 57 accurately describes the state offeePoolafter the migration.x/distribution/module.go (3)
31-31: The downgrade of
ConsensusVersionfrom 5 to 4 is acknowledged and seems intentional based on the previous comments and the summary provided.143-145: Removal of the migration from version 3 to 4 is acknowledged and aligns with the summary provided, which states the deprecation of the
MigrateFundsToPoolfunction.167-169: The modification in the
BeginBlockfunction to callam.keeper.BeginBlocker(c)is noted and appears to be a simplification of the function call.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/distribution/go.mod
Files selected for processing (1)
- CHANGELOG.md (5 hunks)
Additional comments: 5
CHANGELOG.md (5)
85-90: The removal of methods and migration to new modules as part of the AutoCLI migration is clearly documented.
132-137: The documentation of the migration to collections and the removal of methods from the
x/stakingmodule is well-noted.141-147: The changes related to the use of collections, migration of types, and removal of deprecated methods are correctly documented.
153-158: The updates to context creation, coin burning, and the removal of Amino-related functions are accurately captured.
177-187: The changes to command usage and the note on state machine breaking changes are properly documented, including the retraction of a previous release.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/distribution/go.mod
Files selected for processing (1)
- x/distribution/keeper/allocation.go (3 hunks)
Additional comments: 3
x/distribution/keeper/allocation.go (3)
31-40: The summary mentions replacing the direct modification of
feePool.CommunityPoolwithk.FeePool.Set, but the hunk shows modification offeePool.DecimalPool. Please verify if the summary should refer toDecimalPoolinstead ofCommunityPool.67-89: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [24-86]
The summary of the pull request mentions refactoring of the
AllocateTokensfunction to use goroutines and channels for concurrent processing, but the hunk does not show any concurrency-related changes. Please verify if these changes are implemented elsewhere or if the summary is incorrect.
- 152-170: The summary of the pull request mentions a mechanism to transfer funds from the
decimal_poolto the community pool every 1000 blocks, but thesendDecimalPoolToCommunityPoolmethod does not include any logic related to this condition. Please verify if this condition is implemented elsewhere or if the summary is incorrect.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/distribution/go.mod
Files selected for processing (1)
- CHANGELOG.md (5 hunks)
Additional comments: 6
CHANGELOG.md (6)
87-92: The changelog entries for
x/gov/testutil,x/staking/testutil,x/bank/testutil, andappcorrectly document the removal of certain features and migration to new patterns. This aligns with the ongoing efforts to streamline the Cosmos SDK.134-139: The changelog entries for
x/stakingandclientreflect the removal of deprecated methods and the adoption of new patterns, such as using collections forUnbondingTypeandHistoricalInfo. This is consistent with the summary's mention of general code clean-up and module updates.143-149: The changelog entries for
x/staking,x/authz,x/slashing, andtypesdocument the migration of state to collections and the removal of deprecated types and methods. This is in line with the summary's description of changes to various modules and the removal of outdated functionalities.155-160: The changelog entries for
types,x/bank, andclientindicate significant changes to context handling, coin burning, and the removal of Amino-related functions. These changes are part of the broader updates mentioned in the summary, which aim to improve the SDK's efficiency and modernize its codebase.179-185: The changelog entry for
serverandx/auth/vestingdocuments changes to command usage and argument ordering, which are user-facing changes that should be clearly communicated to developers. The entry forx/upgradeandx/stakingreflects structural changes to the state machine, which are critical for developers to understand when upgrading their applications.143-149: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [87-160]
The provided hunks do not include any changes related to the
x/distributionmodule, which is a significant part of the summary. It is important to ensure that all notable changes, especially those involving deprecations and migrations, are documented in the changelog for developers' reference.
Description
I was reading distribution for #18236 (comment) and #18290 and noticed that the fee pool is still used and still accrues.
This PR fully deprecate the community fee pool and let x/protocolpool module account accrue instead.
We do still need a tracker for all those decimals being lost everytime.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Enhancements
Documentation