refactor(x/distribution)!: remove Accounts.String() #19868
Conversation
github-actions
bot
added
C:CLI
C:x/distribution
WalkthroughWalkthroughThe changes focus on transitioning away from global Bech32 prefix usage towards a more modular approach using address codecs for handling account and validator addresses. This involves updating methods to directly accept string parameters, replacing manual address string manipulation with codec utility functions, and ensuring consistent address conversions across different components like messages, keeper operations, and tests. Changes
Related issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
| func CreateValidator(pk cryptotypes.PubKey, stake math.Int) (stakingtypes.Validator, error) { | ||
| valConsAddr := sdk.GetConsAddress(pk) | ||
| val, err := stakingtypes.NewValidator(sdk.ValAddress(valConsAddr).String(), pk, stakingtypes.Description{Moniker: "TestValidator"}) | ||
| func CreateValidator(pk cryptotypes.PubKey, operator string, stake math.Int) (stakingtypes.Validator, error) { |
There was a problem hiding this comment.
Not sure if this should be included in the changelog 😐
![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: 1
Configuration used: .coderabbit.yml
Files selected for processing (18)
- x/distribution/CHANGELOG.md (1 hunks)
- x/distribution/client/common/common.go (1 hunks)
- x/distribution/depinject.go (2 hunks)
- x/distribution/keeper/allocation_test.go (11 hunks)
- x/distribution/keeper/delegation_test.go (30 hunks)
- x/distribution/keeper/genesis.go (8 hunks)
- x/distribution/keeper/grpc_query.go (2 hunks)
- x/distribution/keeper/grpc_query_test.go (3 hunks)
- x/distribution/keeper/keeper.go (1 hunks)
- x/distribution/keeper/keeper_test.go (3 hunks)
- x/distribution/keeper/msg_server_test.go (13 hunks)
- x/distribution/migrations/v4/migrate_funds_test.go (3 hunks)
- x/distribution/simulation/operations.go (2 hunks)
- x/distribution/simulation/proposals.go (1 hunks)
- x/distribution/simulation/proposals_test.go (2 hunks)
- x/distribution/testutil/staking_helper.go (4 hunks)
- x/distribution/types/genesis.go (1 hunks)
- x/distribution/types/msg.go (1 hunks)
Additional comments: 74
x/distribution/client/common/common.go (1)
- 14-19: The conversion from
sdk.AccAddressFromBech32andsdk.ValAddressFromBech32toclientCtx.AddressCodec.StringToBytesfordelegatorAddrandvalidatorAddrrespectively aligns with the PR's objective to utilize more generic and flexible address codec methods. This change enhances consistency and efficiency in address handling.Ensure that the
clientCtx.AddressCodecandclientCtx.ValidatorAddressCodecare properly initialized and configured to handle the address formats expected in this context. This is crucial for maintaining the correctness of address conversions.x/distribution/simulation/proposals.go (1)
- 35-43: The update to the
SimulateMsgUpdateParamsfunction to take acdc coreaddress.Codecparameter and the assignment ofauthorityAddrto theAuthorityfield in theMsgUpdateParamsstruct are in line with the PR's goal of improving address handling. This change ensures that theAuthorityfield is populated with a string representation of the address, which is more consistent and flexible.Ensure that the
cdcparameter passed toSimulateMsgUpdateParamsis properly initialized and capable of performing theBytesToStringconversion as expected. This is important for the correctness of theAuthorityfield assignment.x/distribution/simulation/proposals_test.go (1)
- 33-46: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [23-43]
The addition of the
addressCodecdeclaration usingcodectestutil.CodecOptions{}.GetAddressCodec()and its usage in handling themsgUpdateParams.Authorityaddress in theTestProposalMsgsfunction are consistent with the PR's objectives. This change ensures that address handling in tests aligns with the updated address codec methods used in the main codebase.It's important to verify that the
addressCodecis correctly configured in the test environment to match the address formats used in the main codebase. This ensures that the tests accurately reflect the behavior of the production code.x/distribution/types/msg.go (1)
- 18-21: The modification of the
NewMsgSetWithdrawAddressfunction to acceptstringparameters directly fordelAddrandwithdrawAddris a significant improvement. This change simplifies the function's interface and aligns with the PR's goal of enhancing address handling by directly working with string representations.This update should simplify the usage of
NewMsgSetWithdrawAddressacross the codebase by removing the need for callers to convert addresses to specific types before calling this function. Ensure that all usages of this function have been updated accordingly to pass string parameters.x/distribution/types/genesis.go (1)
- 4-12: The update to the
NewGenesisStatefunction to accept astringinstead of asdk.ConsAddressfor thepp(PreviousProposer) parameter is in line with the PR's objectives of improving address handling. This change simplifies the function's interface and makes it more consistent with the updated approach to address handling.Ensure that all calls to
NewGenesisStatethroughout the codebase have been updated to pass thePreviousProposeras a string. This is important for maintaining consistency and correctness in the initialization of genesis states.x/distribution/depinject.go (1)
- 57-67: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [60-73]
The conversion of the
authorityvariable to a string format usingAddressCodec().BytesToStringmethod before passing it to theNewKeeperconstructor in theProvideModulefunction aligns with the PR's goal of improving address handling. This ensures that theauthorityis handled in a consistent and flexible manner across the module.It's crucial to ensure that the
AddressCodec()is properly initialized and configured to handle the address formats expected in this context. This is important for the correctness of theauthorityconversion and the overall functionality of theNewKeeperconstructor.x/distribution/migrations/v4/migrate_funds_test.go (1)
- 37-60: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [40-91]
The introduction of an
addressCodecvariable and its usage in address handling, particularly for theauthoritydeclaration, in themigrate_funds_test.gofile is consistent with the PR's objectives. This ensures that address handling in tests aligns with the updated address codec methods used in the main codebase.It's important to verify that the
addressCodecis correctly configured in the test environment to match the address formats used in the main codebase. This ensures that the tests accurately reflect the behavior of the production code.x/distribution/testutil/staking_helper.go (3)
- 6-6: The addition of the
cosmossdk.io/core/addressimport is necessary for the updated address handling in theCreateValidatorandDelegatefunctions. This aligns with the PR's objectives of improving address handling across the module.Ensure that this import is used consistently and correctly throughout the file to handle address conversions and operations as intended.
- 15-16: The modification of the
CreateValidatorfunction signature to include anoperator stringparameter and the updated creation logic for theValidatorobject align with the PR's objectives. This change simplifies the function's interface and makes it more consistent with the updated approach to address handling.Ensure that all calls to
CreateValidatorthroughout the test codebase have been updated to pass theoperatoras a string. This is important for maintaining consistency and correctness in the creation of validators for testing purposes.
- 126-139: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [115-136]
The addition of the
addressCodec address.Codecparameter to theDelegatefunction and the updated delegation creation logic are in line with the PR's objectives of improving address handling. This ensures that the delegation process in tests aligns with the updated address codec methods used in the main codebase.It's crucial to ensure that the
addressCodecis properly initialized and configured to handle the address formats expected in this context. This is important for the correctness of the delegation creation and the overall functionality of theDelegatefunction in tests.x/distribution/keeper/grpc_query_test.go (2)
- 58-60: The conversion of validator addresses using
codectestutil.CodecOptions{}.GetValidatorCodec().BytesToString(valConsPk0.Address())is a good practice for ensuring consistency in address handling. This approach aligns with the broader refactor efforts in the module.- 63-63: The conversion of delegator addresses using
codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(addrs[0])is correctly implemented, enhancing the readability and maintainability of the test code by using string representations of addresses.x/distribution/keeper/keeper_test.go (2)
- 42-43: Introducing a
cdcOptsvariable to holdCodecOptionsand using it to create the encoding configuration is a clean and modular approach. This enhances the readability and maintainability of the test setup.- 63-65: The conversion of the
authorityAddrusing the address codec and storing it for later use in theKeeperinitialization is a good practice. It ensures consistency in address handling and improves the clarity of the test setup.x/distribution/keeper/genesis.go (2)
- 162-172: Converting byte slices to strings for delegator and withdraw addresses using the
AddressCodecin theExportGenesisfunction is correctly implemented. This approach aligns with the refactor efforts and ensures consistency in address handling.- 188-194: The conversion of validator addresses to strings in the
ExportGenesisfunction for outstanding rewards records is correctly done, enhancing the readability and maintainability of the code by using string representations of addresses.x/distribution/simulation/operations.go (2)
- 94-103: Converting account addresses to strings before creating a
MsgSetWithdrawAddressmessage is a necessary step for aligning with the broader refactor efforts in the module. This ensures consistency in address handling across different parts of the module.- 154-159: The conversion of delegator addresses to strings before creating a
MsgWithdrawDelegatorRewardmessage is correctly implemented. This approach enhances the readability and maintainability of the simulation code by using string representations of addresses.x/distribution/keeper/keeper.go (1)
- 168-175: The conversion of the withdraw address from bytes to a string before emitting an event is correctly implemented and aligns with the refactor's goals for more generic address handling. Ensure that the
BytesToStringmethod's performance is considered if used frequently in performance-critical paths.x/distribution/keeper/msg_server_test.go (21)
- 14-26: Replacing direct address string manipulation with codec utility functions for converting addresses to strings in tests is a good practice. It aligns with the broader refactor for consistent address handling and improves code readability and maintainability.
- 35-36: Using converted string addresses in message constructors enhances consistency with the main codebase's approach. This change is correctly implemented.
- 44-44: Correctly using the converted string address in the message constructor for the test case "invalid delegator address".
- 51-51: Properly using the converted string address in the test case "invalid withdraw address" maintains consistency in address handling.
- 77-81: The conversion of addresses to strings using codec utility functions in
TestMsgWithdrawDelegatorRewardis correctly applied, enhancing test consistency and readability.- 92-92: Using the converted string address in the message constructor for the test case "invalid delegator address" in
TestMsgWithdrawDelegatorRewardis correctly implemented.- 99-99: Properly using the converted string address in the test case "invalid validator address" in
TestMsgWithdrawDelegatorRewardmaintains consistency in address handling.- 107-108: Correctly using converted string addresses in the message constructor for the test case "no validator" in
TestMsgWithdrawDelegatorReward.- 135-137: The use of codec utility functions for address conversion in
TestMsgWithdrawValidatorCommissionis correctly applied, enhancing test consistency and readability.- 154-154: Correctly using the converted string address in the message constructor for the test case "no validator commission to withdraw" in
TestMsgWithdrawValidatorCommission.- 182-184: Replacing direct address string manipulation with codec utility functions for converting addresses to strings in
TestMsgFundCommunityPoolis a good practice. It aligns with the broader refactor for consistent address handling and improves code readability and maintainability.- 201-201: Using converted string addresses in message constructors enhances consistency with the main codebase's approach. This change is correctly implemented in
TestMsgFundCommunityPool.- 225-229: The conversion of addresses to strings using codec utility functions in
TestMsgUpdateParamsis correctly applied, enhancing test consistency and readability.- 246-246: Properly using the converted string address in the message constructor for the test case "incorrect authority" in
TestMsgUpdateParamsmaintains consistency in address handling.- 254-254: Correctly using the converted string address in the message constructor for the test case "invalid params" in
TestMsgUpdateParams.- 262-262: Using converted string addresses in message constructors enhances consistency with the main codebase's approach. This change is correctly implemented in
TestMsgUpdateParams.- 287-291: The use of codec utility functions for address conversion in
TestMsgCommunityPoolSpendis correctly applied, enhancing test consistency and readability.- 308-308: Properly using the converted string address in the message constructor for the test case "incorrect authority" in
TestMsgCommunityPoolSpendmaintains consistency in address handling.- 316-316: Correctly using the converted string address in the message constructor for the test case "invalid recipient address" in
TestMsgCommunityPoolSpend.- 325-326: Using converted string addresses in message constructors enhances consistency with the main codebase's approach. This change is correctly implemented in
TestMsgCommunityPoolSpend.- 333-334: Correctly using converted string addresses in the message constructor for the test case "success" in
TestMsgCommunityPoolSpend.x/distribution/keeper/grpc_query.go (2)
- 89-93: The conversion of the operator address from bytes to a string in
ValidatorDistributionInfogRPC query is correctly implemented and aligns with the refactor's goals for more generic address handling. This improves consistency and readability in the gRPC queries.- 364-368: The conversion of the withdraw address from bytes to a string in
DelegatorWithdrawAddressgRPC query is correctly implemented. This change aligns with the broader refactor for consistent address handling and enhances the readability and maintainability of the code.x/distribution/keeper/allocation_test.go (5)
- 35-36: The introduction of
cdcOptsto storecodectestutil.CodecOptions{}and its usage in obtaining an address codec is a good practice. It ensures that the codec configuration is centralized and easily modifiable, which enhances maintainability.- 51-53: Converting the module address "gov" to a string using the address codec obtained from
cdcOptsis a necessary change following the removal of theAccounts.String()method. This approach aligns with the PR's objective to use more generic and flexible address codec methods. However, ensure that the "gov" module address is correctly defined and used across the entire module to avoid inconsistencies.Verification successful
The verification process has confirmed that the "gov" module address is consistently used across the module in various contexts, including tests, keeper logic, and other module-specific implementations. This consistency aligns with the objective to use more generic and flexible address codec methods following the removal of the
Accounts.String()method. Therefore, the review comment's concern about ensuring the "gov" module address's consistent use across the entire module is addressed.* 66-68: The conversion of the operator address to a string before creating validator instances is in line with the PR's objective to standardize address handling. This change improves the consistency and flexibility of address handling in the module. Ensure that all instances where validators are created or manipulated are updated to use the new string-based approach for address handling. * 137-139: Similar to the previous comment, converting the operator address to a string for validator creation is consistent with the refactor's goals. It's crucial to verify that this change is applied uniformly across all relevant parts of the module to maintain consistency.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the "gov" module address is consistently used across the module. rg --type go 'NewModuleAddress\("gov"\)'Length of output: 4603
Verification successful
The verification process confirms the uniform application of the string-based approach for address handling in validator creation across the module, as indicated by the consistent use of
distrtestutil.CreateValidatorwith an operator address passed as a string in various test files within thex/distribution/keeperdirectory.* 295-297: The conversion of `valAddr2` to a string using `cdcOpts.GetValidatorCodec().BytesToString(valAddr2)` before creating a validator is consistent with the refactor's objectives. However, this introduces a new method `GetValidatorCodec`, which is not mentioned in the provided context. Ensure that this method is implemented correctly and is part of the `cdcOpts` structure.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Confirm uniform application of the string-based approach for address handling in validator creation across the module. ast-grep --lang go --pattern $'distrtestutil.CreateValidator($_, $_, $_)'Length of output: 2114
Verification successful
Given the initial search did not yield results, let's broaden our search to verify the existence and implementation of the
GetValidatorCodecmethod and to inspect theCodecOptionsstructure. We'll perform two searches: one for any occurrence of theGetValidatorCodecmethod and another for the definition of theCodecOptionsstructure.
The
GetValidatorCodecmethod is correctly implemented within theCodecOptionsstructure, as confirmed by the new context provided. This aligns with the refactor's objectives mentioned in the review comment, verifying the accuracy of the initial observation.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of GetValidatorCodec method in cdcOpts structure. ast-grep --lang go --pattern $'func (CodecOptions) GetValidatorCodec() $_'Length of output: 75
Script:
#!/bin/bash # Broad search for any occurrence of the GetValidatorCodec method ast-grep --lang go --pattern $'func ($_ CodecOptions) GetValidatorCodec() $_' # Search for the definition of the CodecOptions structure ast-grep --lang go --pattern $'type CodecOptions struct { $$$ }'Length of output: 1100
x/distribution/keeper/delegation_test.go (27)
- 47-48: The conversion from
ModuleAddressto string usingBytesToStringis correctly handled, but ensure that the error is checked immediately after the function call to adhere to best practices in error handling.- 68-69: The conversion of validator addresses using
BytesToStringis correctly implemented. This change aligns with the PR's objective to standardize address handling.- 74-77: The conversion of account and validator addresses to strings using
BytesToStringis correctly done. This is part of the broader refactor to improve address manipulation consistency.- 160-161: Repeating the conversion of
ModuleAddressto string across different test functions. While this repetition is somewhat expected in test files, consider using a helper function if the pattern is prevalent across many tests to reduce boilerplate code.- 183-184: Correct usage of
BytesToStringfor address conversion in the context of testing slashing effects on rewards. This consistency in address handling is crucial for the refactor's goals.- 189-192: The address conversion logic is consistently applied, which is good for maintaining the uniform approach to address handling throughout the module as mentioned in the PR summary.
- 276-277: The repeated conversion logic for
ModuleAddressto string is noted. As mentioned earlier, consider a helper function for reuse in tests to streamline the code.- 299-300: The use of
BytesToStringfor converting validator addresses is consistent with the refactor's objectives. This consistency is key to the improvements in the module.- 305-308: The address conversion for both account and validator addresses is correctly implemented, aligning with the refactor's aim for improved address handling.
- 413-414: Again, the conversion from
ModuleAddressto string is seen. The repetition across tests underscores the potential benefit of a helper function for such conversions.- 434-435: The consistent application of
BytesToStringfor address conversion in tests reflects the PR's goal of standardizing address handling.- 439-441: Correct implementation of address conversion, which is part of the broader efforts to standardize and simplify address handling in the module.
- 523-524: The repeated pattern of converting
ModuleAddressto string is observed. Consideration of a helper function for such repetitive tasks could improve code maintainability.- 544-545: The use of
BytesToStringfor converting validator addresses is consistent across tests, aligning with the refactor's objectives for address handling.- 551-554: The address conversion logic for both account and validator addresses is correctly implemented, supporting the refactor's aim for consistent address handling.
- 611-612: The pattern of converting
ModuleAddressto string is repeated across multiple tests. A helper function could reduce redundancy and improve code clarity.- 632-633: Consistent application of
BytesToStringfor address conversion in the context of testing various scenarios, aligning with the refactor's goals.- 639-642: Correct implementation of address conversion, reflecting the PR's objective to standardize address handling across the module.
- 740-741: The repeated conversion of
ModuleAddressto string across tests is noted. A helper function could streamline these conversions and reduce code duplication.- 763-764: The use of
BytesToStringfor converting validator addresses is correctly implemented, reflecting the PR's aim for consistent and improved address handling.- 769-772: The address conversion for both account and validator addresses is correctly done, aligning with the refactor's objectives for consistent address handling.
- 893-894: The pattern of converting
ModuleAddressto string is repeated. As previously mentioned, a helper function could improve code maintainability by reducing redundancy.- 914-915: Consistent use of
BytesToStringfor address conversion across tests, aligning with the refactor's goals for standardized address handling.- 920-923: Correct implementation of address conversion, supporting the broader efforts to standardize address handling in the module.
- 1107-1108: The repeated conversion of
ModuleAddressto string across tests underscores the potential benefit of a helper function for such conversions to improve code clarity.- 1128-1129: The consistent application of
BytesToStringfor converting validator addresses is noted, aligning with the refactor's objectives.- 1134-1137: The address conversion logic for both account and validator addresses is correctly implemented, reflecting the PR's aim for consistent and improved address handling.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (2)
- x/distribution/CHANGELOG.md (1 hunks)
- x/distribution/client/common/common_test.go (2 hunks)
Additional comments: 3
x/distribution/client/common/common_test.go (3)
- 4-4: The addition of
codectestutilis appropriate for supporting the new codec options used in the test. Good inclusion for enhanced testing capabilities.- 14-18: Initialization of
cdcOptsand its use in configuringclientCtxwith specific codec options aligns well with the PR's objectives. This enhances test reliability by ensuring the correct codecs are used.- 11-21: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [19-33]
The test cases are well-designed to validate the address validation logic in
QueryDelegationRewards, covering scenarios with invalid and empty delegator and validator addresses. This ensures robust error handling, aligning with the refactor's objectives.
| * [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method | ||
| * `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`. | ||
| * `NewGenesisState` now takes a string as argument instead of `sdk.ConsAddress`. |
There was a problem hiding this comment.
Please add a space between sentences for better readability. Also, ensure that sentences start with an uppercase letter for consistency.
- * `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`.
+ * `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`.
+ * `NewGenesisState` now takes a string as argument instead of `sdk.ConsAddress`.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.
| * [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method | |
| * `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`. | |
| * `NewGenesisState` now takes a string as argument instead of `sdk.ConsAddress`. | |
| * [#19868](https://github.com/cosmos/cosmos-sdk/pull/19868) Removes Accounts String method | |
| * `NewMsgSetWithdrawAddress` now takes strings as argument instead of `sdk.AccAddress`. | |
| * `NewGenesisState` now takes a string as argument instead of `sdk.ConsAddress`. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/distribution/client/common/common_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/distribution/client/common/common_test.go
Co-authored-by: son trinh <trinhleson2000@gmail.com>
Description
ref:
#13140
#7448
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit