refactor(x/staking)!: removes the use of Accounts String() method #19742
Conversation
github-actions
bot
added
C:x/staking
C:CLI
C:x/gov
C:x/slashing
C:x/distribution
WalkthroughThe updates aim to enhance the Cosmos SDK by integrating address and validator codecs across various modules and tests. These changes ensure consistent handling of address conversions and validations, improving the SDK's robustness and functionality. By adding codec parameters to function calls and struct initializations, the modifications facilitate more flexible and error-resistant address handling within the Cosmos ecosystem. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (56)
- simapp/sim_bench_test.go (1 hunks)
- simapp/sim_test.go (5 hunks)
- tests/integration/evidence/keeper/infraction_test.go (2 hunks)
- tests/integration/slashing/keeper/keeper_test.go (3 hunks)
- tests/sims/gov/operations_test.go (2 hunks)
- testutil/sims/simulation_helpers.go (1 hunks)
- testutil/sims/state_helpers.go (6 hunks)
- types/module/simulation.go (2 hunks)
- types/simulation/types.go (2 hunks)
- x/auth/simulation/genesis_test.go (2 hunks)
- x/auth/simulation/proposals.go (3 hunks)
- x/auth/simulation/proposals_test.go (2 hunks)
- x/authz/client/cli/tx.go (1 hunks)
- x/authz/msgs_test.go (3 hunks)
- x/authz/simulation/genesis_test.go (1 hunks)
- x/bank/simulation/genesis_test.go (2 hunks)
- x/bank/simulation/proposals.go (3 hunks)
- x/bank/simulation/proposals_test.go (2 hunks)
- x/distribution/simulation/genesis_test.go (2 hunks)
- x/distribution/simulation/proposals.go (3 hunks)
- x/distribution/simulation/proposals_test.go (2 hunks)
- x/evidence/simulation/genesis_test.go (2 hunks)
- x/feegrant/simulation/genesis_test.go (1 hunks)
- x/gov/simulation/genesis_test.go (2 hunks)
- x/gov/simulation/operations.go (1 hunks)
- x/gov/simulation/proposals.go (2 hunks)
- x/gov/simulation/proposals_test.go (2 hunks)
- x/group/simulation/genesis_test.go (1 hunks)
- x/mint/simulation/genesis_test.go (1 hunks)
- x/mint/simulation/proposals.go (3 hunks)
- x/mint/simulation/proposals_test.go (2 hunks)
- x/nft/simulation/genesis_test.go (1 hunks)
- x/protocolpool/simulation/proposals.go (3 hunks)
- x/protocolpool/simulation/proposals_test.go (2 hunks)
- x/slashing/simulation/genesis_test.go (2 hunks)
- x/slashing/simulation/proposals.go (3 hunks)
- x/slashing/simulation/proposals_test.go (2 hunks)
- x/staking/keeper/alias_functions.go (1 hunks)
- x/staking/keeper/cons_pubkey_test.go (2 hunks)
- x/staking/keeper/delegation_test.go (31 hunks)
- x/staking/keeper/keeper_test.go (10 hunks)
- x/staking/keeper/msg_server_test.go (61 hunks)
- x/staking/keeper/validator_test.go (3 hunks)
- x/staking/migrations/v5/migrations_test.go (3 hunks)
- x/staking/simulation/genesis.go (2 hunks)
- x/staking/simulation/genesis_test.go (2 hunks)
- x/staking/simulation/operations.go (4 hunks)
- x/staking/simulation/proposals.go (3 hunks)
- x/staking/simulation/proposals_test.go (3 hunks)
- x/staking/testutil/helpers.go (3 hunks)
- x/staking/testutil/validator.go (1 hunks)
- x/staking/types/authz.go (4 hunks)
- x/staking/types/authz_test.go (25 hunks)
- x/staking/types/delegation_test.go (4 hunks)
- x/staking/types/msg_test.go (2 hunks)
- x/staking/types/validator_test.go (4 hunks)
Check Runs (11)
tests (03) completed (1)
tests (01) completed (1)
test-x-mint completed (1)
test-x-bank completed (1)
test-x-authz completed (1)
test-x-slashing completed (1)
test-x-gov completed (1)
test-x-auth in_progress (1)
test-x-upgrade completed (1)
test-x-params completed (1)
test-x-group completed (1)
Additional comments: 132
x/staking/testutil/validator.go (1)
- 18-19: The use of
require.NoError(tb, err)for error handling in tests is generally acceptable as it will halt the test execution if an error is encountered. However, consider if there's a need for more nuanced error handling or logging in this context, especially for more complex test setups where you might want to clean up resources or provide more detailed error messages.x/bank/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddressimport is appropriate for the changes made in theSimulateMsgUpdateParamsfunction.- 34-34: The modification to include an additional parameter
addressCodec coreaddress.Codecand changing the return type to(sdk.Msg, error)in theSimulateMsgUpdateParamsfunction enhances error handling and aligns with the PR's objectives for improved address handling.x/protocolpool/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddressimport is appropriate for the changes made in theSimulateMsgCommunityPoolSpendfunction.- 31-31: The modification to include an additional parameter
addressCodec coreaddress.Codecand changing the return type to(sdk.Msg, error)in theSimulateMsgCommunityPoolSpendfunction enhances error handling and aligns with the PR's objectives for improved address handling.x/distribution/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddressimport is appropriate for the changes made in theSimulateMsgUpdateParamsfunction.- 35-35: The modification to include an additional parameter
addressCodec coreaddress.Codecand changing the return type to(sdk.Msg, error)in theSimulateMsgUpdateParamsfunction enhances error handling and aligns with the PR's objectives for improved address handling.x/bank/simulation/proposals_test.go (2)
- 12-12: The addition of the
codectestutilimport is appropriate for the changes made in theTestProposalMsgsfunction, providing necessary codec utilities for testing.- 36-36: The modification to include an additional argument
codectestutil.CodecOptions{}.GetAddressCodec()in theTestProposalMsgsfunction aligns with the PR's objectives for improved address handling and ensures consistency between the main codebase and tests.x/distribution/simulation/proposals_test.go (2)
- 13-13: The addition of the
codectestutilimport is appropriate for the changes made in theTestProposalMsgsfunction, providing necessary codec utilities for testing.- 37-37: The modification to include an additional argument
codectestutil.CodecOptions{}.GetAddressCodec()in theTestProposalMsgsfunction aligns with the PR's objectives for improved address handling and ensures consistency between the main codebase and tests. The addition of an error check for message creation is a good practice for robust testing.x/feegrant/simulation/genesis_test.go (1)
- 25-37: The updates to the
SimulationStatestruct initialization in theTestRandomizedGenStatefunction, including theAddressCodecandValidatorCodecfields, align with the PR's objectives for improved address handling. This ensures consistency between the main codebase and simulation tests.x/protocolpool/simulation/proposals_test.go (2)
- 12-12: The addition of the
codectestutilimport is appropriate for the changes made in theTestProposalMsgsfunction, providing necessary codec utilities for testing.- 36-36: The modification to include an additional argument
codectestutil.CodecOptions{}.GetAddressCodec()in theTestProposalMsgsfunction aligns with the PR's objectives for improved address handling and ensures consistency between the main codebase and tests.x/evidence/simulation/genesis_test.go (2)
- 26-26: The introduction of
cdcOptsastestutil.CodecOptions{}is a good practice for managing codec options. This ensures that any codec-related configurations are centralized and easily modifiable.- 33-34: Adding
AddressCodecandValidatorCodecto theSimulationStatestruct is crucial for handling address conversions more robustly. This change aligns with the PR's objective to improve address handling across the SDK.x/authz/simulation/genesis_test.go (1)
- 32-33: The inclusion of
AddressCodecandValidatorCodecin theSimulationStatestruct is a significant improvement. It ensures that address handling is consistent and error-resistant across different modules of the SDK.x/nft/simulation/genesis_test.go (2)
- 23-24: The introduction of
cdcOptsand its use inMakeTestEncodingConfigis a good practice. It allows for more flexible and configurable codec options, which can be beneficial for testing different encoding scenarios.- 32-33: The assignment of
AddressCodecandValidatorCodecusingcdcOptsis a positive change. It ensures that address handling is consistent and robust, aligning with the PR's objectives to improve the SDK's code quality.x/auth/simulation/proposals_test.go (1)
- 36-37: The addition of
codectestutil.CodecOptions{}.GetAddressCodec()as an argument in theMsgSimulatorFncall and the introduction of error handling are significant improvements. They ensure that address handling is consistent with the new codec approach and that potential errors are properly managed.x/auth/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddresspackage import is necessary for utilizing the new codec approach for address handling. This aligns with the PR's objectives to improve address handling across the SDK.- 31-37: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [34-48]
Modifying the
SimulateMsgUpdateParamsfunction to include anaddressCodec coreaddress.Codecparameter and changing the return type to(sdk.Msg, error)are crucial changes. They ensure that the function is compatible with the new address handling approach and that potential errors are properly managed.x/gov/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddresspackage import is a necessary step for utilizing the new codec approach for address handling within the government simulation proposals. This aligns with the broader objectives of improving address handling across the SDK.- 30-31: The modification of the
SimulateTextProposalfunction to include anaddressCodec coreaddress.Codecparameter, even though it currently returnsnil, nil, prepares the function for future enhancements where address handling might be required.x/slashing/simulation/proposals_test.go (1)
- 38-39: The addition of
codectestutil.CodecOptions{}.GetAddressCodec()as an argument in theMsgSimulatorFncall and the introduction of error handling are important improvements. They ensure that address handling is consistent with the new codec approach and that potential errors are properly managed.x/slashing/simulation/proposals.go (2)
- 7-7: The addition of the
coreaddresspackage import is necessary for utilizing the new codec approach for address handling within the slashing module. This aligns with the broader objectives of improving address handling across the SDK.- 33-39: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-50]
Modifying the
SimulateMsgUpdateParamsfunction to include anaddressCodec coreaddress.Codecparameter and changing the return type to(sdk.Msg, error)are crucial changes. They ensure that the function is compatible with the new address handling approach and that potential errors are properly managed.x/mint/simulation/proposals_test.go (2)
- 13-13: The addition of the
codectestutilpackage import is appropriate for the changes made in this file, specifically for handling address encoding/decoding.- 37-37: The modification to include
codectestutil.CodecOptions{}.GetAddressCodec()as an additional parameter in theMsgSimulatorFncall aligns with the PR's objectives to enhance address handling. Ensure that all related function calls across the codebase are updated to include this new parameter.x/mint/simulation/proposals.go (2)
- 6-6: The addition of the
coreaddresspackage import supports the PR's goal of improving address handling through enhanced codec utilities.- 35-35: The update to
SimulateMsgUpdateParamsto include anaddressCodec coreaddress.Codecparameter and return(sdk.Msg, error)is a positive change, enhancing address handling and error management. Ensure that all calls to this function are updated to handle the new return type properly.x/staking/simulation/proposals.go (2)
- 7-7: The import of
coreaddressis appropriate for the changes in this file, facilitating improved address handling through codec utilities.- 36-36: The modifications to
SimulateMsgUpdateParams, including the addition of anaddressCodec coreaddress.Codecparameter and improved error handling, align with the PR's objectives for better address management. Ensure proper error handling is implemented wherever this function is called.x/staking/simulation/proposals_test.go (2)
- 14-14: The addition of the
codectestutilpackage import is appropriate for the changes in this file, specifically for handling address encoding/decoding.- 27-27: The initialization of
addressCodecand its use in theTestProposalMsgsfunction align with the PR's objectives to enhance address handling through codec utilities. Ensure that address handling is consistent across all tests.x/staking/types/msg_test.go (2)
- 12-12: The addition of the
codectestutilpackage import supports the PR's goal of enhancing codec handling, specifically for validator codec utilities in this test.- 38-38: The introduction of
vAddr1and its use in theNewMsgCreateValidatorcall within theTestMsgDecodefunction align with the PR's objectives for improved address handling through codec utilities. Ensure consistent address handling is maintained in all relevant tests.x/group/simulation/genesis_test.go (1)
- 25-39: The updates to the
SimulationStatestruct in theTestRandomizedGenStatefunction, including the addition ofAddressCodecandValidatorCodecfields, align with the PR's objectives to standardize address handling using codec utilities. Ensure consistent codec handling is applied across all simulation tests.x/auth/simulation/genesis_test.go (1)
- 27-40: The inclusion of
AddressCodecandValidatorCodecfields in theSimulationStatestruct during its initialization in theTestRandomizedGenStatefunction supports the PR's goal of enhancing codec handling for address and validator information. Ensure that codec handling is consistently applied across all simulation tests.x/distribution/simulation/genesis_test.go (1)
- 26-39: The updates to the
SimulationStatestruct in theTestRandomizedGenStatefunction, including the addition ofAddressCodecandValidatorCodecfields, align with the PR's objectives to standardize codec handling across the SDK. Ensure consistent codec handling is maintained in all simulation tests.simapp/sim_bench_test.go (1)
- 69-69: The addition of
AuthKeeper.AddressCodec()andStakingKeeper.ValidatorAddressCodec()as parameters toAppStateFnaligns with the refactor's goal to improve address handling. Ensure the implementation and usage of these codecs are consistent and error-free across the SDK.x/slashing/simulation/genesis_test.go (1)
- 27-27: The initialization of
AddressCodecandValidatorCodecusingcdcOptsin theSimulationStatestruct is a good practice for standardizing address handling. Verify the correct usage of these codecs in simulations and tests to ensure consistency and prevent issues.x/bank/simulation/genesis_test.go (1)
- 28-28: The creation of
cdcOptsto handle codec options for testing, and its use in initializingAddressCodecandValidatorCodecin theSimulationStatestruct, is a positive step towards improving address handling. Ensure the correct usage of these codecs in the context of bank module simulations and tests.x/mint/simulation/genesis_test.go (1)
- 25-25: The introduction of
cdcOptsforCodecOptionsand its use in initializingAddressCodecandValidatorCodecin theSimulationStatestruct is a good practice for standardizing address handling within the mint module. Verify the correct usage of these codecs in simulations and tests to ensure consistency and prevent issues.x/gov/simulation/genesis_test.go (1)
- 29-29: The initialization of
cdcOptsfor codec options and its use in settingAddressCodecandValidatorCodecin theSimulationStatestruct is a good practice for standardizing address handling within the gov module. Verify the correct usage of these codecs in simulations and tests to ensure consistency and prevent issues.x/staking/simulation/genesis.go (2)
- 79-82: Converting
ValAddressto strings usingValidatorCodecand introducingValStrAddressfor use inNewValidatorandNewDelegationcalls aligns with efforts to standardize address handling in the staking module. Ensure the correct implementation and usage of these conversions to prevent issues.- 100-104: Similarly, converting
AccAddressto strings usingAddressCodecfor use inNewDelegationis a good practice. Verify the correct implementation and usage of these conversions to ensure consistency and prevent issues in the staking module.x/staking/keeper/alias_functions.go (1)
- 62-66: Using
validatorAddressCodec.BytesToString(address)for converting validator addresses to strings inIterateBondedValidatorsByPowerimproves error handling by providing more informative error messages. Ensure the correct implementation and usage of this method across the keeper functions to maintain consistency and prevent issues.x/staking/migrations/v5/migrations_test.go (1)
- 93-105: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [84-102]
The introduction of
codecOptsfor codec options and its use in converting validator and account addresses to strings in migration tests aligns with efforts to standardize address handling within the staking module. Verify the correct implementation and usage of these codecs in the context of staking module migrations and tests to ensure consistency and prevent issues.x/staking/simulation/genesis_test.go (2)
- 29-29: The introduction of
cdcOptsfor codec options is a good practice for managing codec configurations. This change enhances the flexibility and maintainability of codec usage within tests.- 37-38: Using
AddressCodecandValidatorCodecwithin theSimulationStatestruct is a significant improvement. It ensures that address handling is consistent and error-resistant across the simulation tests. This change aligns with the PR's objective to improve address handling within the Cosmos SDK.x/staking/testutil/helpers.go (4)
- 56-58: Converting
sdk.ValAddresstostringusing theValidatorAddressCodecand handling potential errors withrequire.NoErroris a robust approach. This ensures that address conversions are handled safely and errors are properly checked during tests.- 69-71: Similar to the previous comment, the use of
ValidatorAddressCodecfor address conversion and error checking withrequire.NoErrorin thecreateValidatorfunction is a good practice. It maintains consistency and reliability in address handling.- 84-86: The changes in the
Delegatefunction, including the direct use ofstakingtypes.NewMsgDelegatewithout address conversion, seem appropriate given the context. However, it's important to ensure that thedelegatorandvalparameters are correctly formatted as strings elsewhere in the code.- 102-104: The
Undelegatefunction's modifications follow the same pattern as theDelegatefunction, focusing on message creation without direct address conversion in this context. Consistency in handling these operations is crucial for maintainability.x/staking/types/delegation_test.go (6)
- 21-25: Introducing
codecOptsand utilizing it for address and validator codec operations in tests is a commendable approach. It ensures that address conversions are handled in a standardized and error-resistant manner, aligning with the PR's objectives.- 32-34: The use of
codecOptsfor address conversion in theTestDelegationEqualfunction is consistent with the overall improvements in address handling. This enhances the reliability of tests by ensuring proper address formatting.- 42-47: Applying
codecOptsin theTestDelegationStringfunction for address conversions further demonstrates the PR's commitment to improving address handling within the SDK. This change contributes to the maintainability and readability of the code.- 59-61: Utilizing
codecOptsfor address conversion in theTestUnbondingDelegationEqualfunction is in line with the PR's goals. It ensures that address handling is consistent and reduces the potential for errors.- 99-103: The introduction of
codecOptsin theTestDelegationResponsesfunction and its use for address conversions is a positive change. It standardizes address handling across tests, contributing to the code's overall quality.- 134-145: In the
TestRedelegationResponsesfunction, the addition ofcodecOptsand its application for address and validator codec operations is a significant improvement. It ensures that address conversions are handled in a consistent and error-proof manner.types/simulation/types.go (2)
- 11-12: Adding the
cosmossdk.io/core/addressimport is necessary for the subsequent changes in the file. This ensures that the address codec functionality is available for use in simulation types, aligning with the broader goals of improving address handling.- 44-44: Modifying the signature of the
MsgSimulatorFnfunction to include an additional parametercdc address.Codecis a strategic improvement. It allows for more flexible and error-resistant address handling within simulation functions, contributing to the overall code quality.x/authz/msgs_test.go (2)
- 58-58: The addition of
codectestutiland the use ofvalAddressCodecin theNewStakeAuthorizationfunction call represent a thoughtful approach to improving the testability and correctness of authorization logic. This ensures that validator addresses are handled correctly in the context of authorization tests.- 77-77: Incorporating
valAddressCodecinto theNewStakeAuthorizationfunction enhances the robustness of the authorization logic by ensuring that validator addresses are converted and handled properly. This change aligns with the PR's objectives to improve address handling.x/staking/keeper/cons_pubkey_test.go (3)
- 32-34: Converting validator addresses from bytes to string using the
ValidatorAddressCodecand handling potential errors withrequire.NoErrorin theTestConsPubKeyRotationHistoryfunction is a good practice. It ensures that address conversions are handled safely and errors are properly checked during tests.- 161-163: Similar to previous comments, the use of
ValidatorAddressCodecfor address conversion and error checking withrequire.NoErrorin thesetValidatorsfunction is a robust approach. It maintains consistency and reliability in address handling.- 171-174: The creation of
NewDelegationobjects with modified parameters, including the conversion of validator addresses, in thesetValidatorsfunction is consistent with the PR's objectives. This change enhances the maintainability and readability of the code.x/gov/simulation/proposals_test.go (2)
- 11-11: The addition of
codectestutilimport is aligned with the PR's objective to improve address handling by using codec utilities. This change supports the modifications in theTestProposalMsgsfunction to use theAddressCodec.- 34-35: The modification in the
TestProposalMsgsfunction to includecodectestutil.CodecOptions{}.GetAddressCodec()as an argument in theMsgSimulatorFncall is a direct implementation of the PR's objective to avoid direct string conversions of addresses. This change, along with the added error handling, enhances the robustness of the address handling in tests.However, it's important to ensure that the error handling pattern is consistent across the test suite. The use of
assert.NilError(t, err)directly after the function call that could produce an error is a good practice, ensuring that tests fail fast on unexpected errors.One minor suggestion is to add a comment explaining the use of
codectestutil.CodecOptions{}.GetAddressCodec()for future maintainers or contributors who might not be familiar with the context of this change.Consider adding a brief comment explaining the use of
codectestutil.CodecOptions{}.GetAddressCodec()to improve code readability and maintainability.x/staking/types/authz.go (2)
- 20-21: The addition of the
valAddressCodec address.Codecparameter in theNewStakeAuthorizationfunction is a significant improvement towards achieving the PR's objective of enhancing address handling. This change ensures that address conversions within the function are handled more robustly and error-proof.It's important to ensure that all calls to
NewStakeAuthorizationthroughout the codebase have been updated to include the newvalAddressCodecparameter. This change might require modifications in other parts of the Cosmos SDK whereNewStakeAuthorizationis used.Verification successful
The verification process confirms that all calls to
NewStakeAuthorizationthroughout the codebase have been updated to include the newvalAddressCodecparameter. This ensures compatibility with the updated function signature and aligns with the objective of enhancing address handling. The changes are consistent across tests, client CLI commands, and other usages, indicating a comprehensive update.* 167-197: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [157-194]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to NewStakeAuthorization to ensure the new parameter is included. rg --type go 'NewStakeAuthorization\('Length of output: 3003
The modifications in the
validateAllowAndDenyValidatorsfunction to include thevalAddressCodec address.Codecparameter and use it for address conversions are well-aligned with the PR's objectives. This change enhances the robustness of address handling by utilizing the codec for conversions instead of relying on direct string conversions.The error handling for address conversion failures (
sdkerrors.ErrInvalidRequest.Wrap("could not convert validator address")) is appropriate and ensures that the function fails gracefully in case of conversion issues.One suggestion is to ensure that the error messages are as informative as possible. For instance, including the invalid address in the error message could help with debugging.
Consider including the invalid address in the error message for conversion failures to improve debuggability.
- sdkerrors.ErrInvalidRequest.Wrap("could not convert validator address") + sdkerrors.ErrInvalidRequest.Wrapf("could not convert validator address: %s", validator.String())Note: Ensure that
validator.String()does not reintroduce the direct string conversion issue the PR aims to solve.testutil/sims/simulation_helpers.go (1)
- 57-61: The addition of
AddressCodecandValidatorCodecto theSimulationStatestruct and their usage inauthtx.NewTxConfigis a positive change. It aligns with the PR's goal to improve address handling by using codecs instead of direct string conversions. This should enhance maintainability and reduce potential errors.types/module/simulation.go (2)
- 9-9: The import of the
cosmossdk.io/core/addresspackage is necessary for utilizing theaddress.Codectype in theSimulationStatestruct. This is a good practice as it leverages theaddresspackage for more robust address handling.- 149-150: Adding
AddressCodecandValidatorCodecof typeaddress.Codecto theSimulationStatestruct is a significant improvement. It ensures that address handling is consistent and error-resistant across simulation operations, aligning with the PR's objectives.x/authz/client/cli/tx.go (3)
- 214-214: Adding
clientCtx.ValidatorAddressCodecas an argument to theNewStakeAuthorizationfunction call for thedelegatecase is a good practice. It ensures that address handling is consistent and leverages the new codec approach.- 216-216: Similarly, for the
unbondcase, the inclusion ofclientCtx.ValidatorAddressCodecin theNewStakeAuthorizationfunction call aligns with the PR's objectives to improve address handling.- 218-218: For the
redelegatecase, addingclientCtx.ValidatorAddressCodecto theNewStakeAuthorizationfunction call is consistent with the refactor's goal. It ensures that address handling uses the new codec approach.testutil/sims/state_helpers.go (5)
- 15-15: The import of the
cosmossdk.io/core/addresspackage is necessary for the newaddressCodecandvalidatorCodecparameters introduced in various functions. This aligns with the PR's goal of improving address handling.- 40-41: The addition of
addressCodecandvalidatorCodecparameters to theAppStateFnfunction signature is a positive change. It ensures that address handling is consistent and leverages the new codec approach across simulation helpers.- 48-53: Similarly, the inclusion of
addressCodecandvalidatorCodecin theAppStateFnWithExtendedCbfunction signature aligns with the refactor's goal. It ensures that address handling uses the new codec approach.- 106-116: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [64-113]
The changes in
AppStateFnWithExtendedCbsandAppStateRandomizedFnto includeaddressCodecandvalidatorCodecparameters, and the update to theSimulationStatestruct to include these codecs, are significant improvements. They ensure that address handling is consistent and error-resistant across simulation operations.
- 230-246: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [201-243]
The instantiation of
SimulationStatewithAddressCodecandValidatorCodecinAppStateRandomizedFnis a crucial update. It aligns with the PR's objectives to improve address handling by using codecs instead of direct string conversions, enhancing maintainability and reducing potential errors.x/staking/types/validator_test.go (2)
- 18-18: The addition of
codectestutilimport is appropriate for enhancing testing capabilities related to encoding and decoding. This aligns well with the PR's objectives to improve address handling.- 338-346: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [140-356]
The changes to use
codectestutilfor encoding and decoding addresses in tests are well-aligned with the PR's objectives to avoid direct string conversions of addresses. This approach enhances consistency and reduces the potential for errors, which is a good practice.simapp/sim_test.go (4)
- 89-89: The inclusion of additional parameters for address codecs in the
AppStateFnfunction call withinTestFullAppSimulationis a positive change. It ensures more comprehensive codec handling for addresses, aligning with the PR's objectives to improve address handling.- 137-137: Similar to
TestFullAppSimulation, the update inTestAppImportExportto include additional parameters for address codecs in theAppStateFnfunction call is well-justified. It enhances the simulation setup's ability to handle addresses more effectively.- 258-258: The changes in
TestAppSimulationAfterImport, including additional parameters for address codecs in theAppStateFnfunction call, are consistent with the PR's objectives. This ensures a more robust simulation setup for address handling.- 389-389: Including additional parameters for address codecs in the
AppStateFnfunction call withinTestAppStateDeterminismaligns with the PR's goals to improve address handling. This is a commendable update to the simulation tests.x/staking/types/authz_test.go (3)
- 28-40: The addition of
valAddressToStringandaccAddressToStringhelper functions improves code readability and maintainability by centralizing address conversion logic. Ensure that the codec utilities (GetValidatorCodecandGetAddressCodec) used here are correctly implemented and available in the test context.- 46-46: The introduction of
valAddressCodecfor address conversion in tests aligns with the PR's objectives. Ensure that it is consistently used across tests and correctly initialized.- 25-84: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [48-372]
Updates to test cases to use the new helper functions for address conversion and the
valAddressCodecparameter are consistent with the PR's objectives. Verify that these changes do not alter the intended behavior of the tests and that all necessary cases are covered.tests/integration/evidence/keeper/infraction_test.go (2)
- 264-268: Modifications to
TestHandleDoubleSignto use address conversion before callingtstaking.Undelegatealign with the PR's objectives. Ensure that the address codecs used are correctly implemented and available in the test context.- 431-435: Modifications to
TestHandleDoubleSignAfterRotationto use address conversion before callingtstaking.Undelegateare consistent with the PR's objectives. Verify the correct implementation and availability of the address codecs used.x/staking/keeper/keeper_test.go (7)
- 72-73: The conversion from
ModuleAddressto string usingBytesToStringis a good approach to avoid direct string manipulation. However, ensure that theauthorityvariable is used consistently and securely throughout the codebase.- 97-101: The
addressToStringfunction correctly utilizes theaccountKeeper'sAddressCodecfor converting addresses to strings. This is a safer approach compared to direct string conversions.- 103-107: Similarly, the
valAddressToStringfunction uses thestakingKeeper'sValidatorAddressCodecfor address conversion. This consistency in handling address conversions is commendable.- 364-365: The use of
addressToStringandvalAddressToStringwithin theTestUnbondingDelegationsMigrationToCollstest function for settingDelegatorAddressandValidatorAddressfields inUnbondingDelegationinstances is a good practice. It ensures that address conversions are handled through the newly introduced functions, enhancing code maintainability and readability.Also applies to: 389-390
- 451-451: In the
TestValidatorsMigrationToCollstest function, the conversion of validator addresses usingvalAddressToStringbefore setting theOperatorAddressfield inValidatorinstances is consistent with the PR's objective to standardize address handling. This change improves the code's robustness against potential errors in direct string conversions.Also applies to: 477-477
- 509-509: The conversion of validator addresses to strings using
valAddressToStringin theTestValidatorQueueMigrationToCollstest function before appending them to theaddrsslice is a good practice. It aligns with the overall goal of the PR to standardize and secure address handling across the codebase.Also applies to: 526-526
- 549-551: The use of
addressToStringandvalAddressToStringfor converting delegator and validator addresses in theTestRedelegationQueueMigrationToCollstest function is consistent with the PR's objectives. This approach ensures that address conversions are handled in a standardized manner, reducing the risk of errors.Also applies to: 572-574
tests/sims/gov/operations_test.go (2)
- 12-12: The addition of
"cosmossdk.io/core/address"is aligned with the PR's objective to improve address handling by using more robust methods instead of direct string conversions. This import is necessary for utilizingaddress.Codecin the modified function signature.- 59-60: The modification of the
MsgSimulatorFnfunction signature to include an additional parameteraddress.Codecis a direct implementation of the PR's objective to avoid direct string conversions of addresses. This change ensures that address handling within simulations leverages theaddress.Codecfor more robust and error-resistant conversions. However, it's important to ensure that all calls to this function across the codebase are updated to pass the necessaryaddress.Codecparameter.Verification successful
The verification process confirms that the
MsgSimulatorFnfunction signature change, which includes an additional parameteraddress.Codec, has been correctly implemented and used across the codebase. All calls to this function now pass the necessaryaddress.Codecparameter, aligning with the PR's objective to enhance address handling within simulations. This ensures that the modification is consistent with the intended improvements in address conversion robustness and error resistance.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MsgSimulatorFn usage across the codebase to ensure the new signature is correctly used. rg --type go 'MsgSimulatorFn\('Length of output: 1687
x/gov/simulation/operations.go (1)
- 158-161: The addition of error handling for the
msgSimfunction call is a positive change, enhancing the robustness of the simulation logic. It's good practice to handle errors explicitly rather than ignoring them, which this change accomplishes.However, consider adding more context to the error message returned on line 160 to help with debugging. For instance, prefixing the error with "SimulateMsgSubmitProposal: failed to generate proposal message:" could provide clearer insights into the source of the error when it occurs.
tests/integration/slashing/keeper/keeper_test.go (3)
- 198-202: The conversion of validator addresses using
AddressCodecandValidatorAddressCodecis a key part of the refactor. Ensure that theBytesToStringmethod correctly handles the address formats specific to the Cosmos SDK. This change enhances readability and maintainability by using more descriptive methods for address handling.- 227-232: The delegation logic to meet minimum self-delegation is correctly updated to use the new address conversion methods. This ensures consistency in address handling throughout the test, aligning with the PR's objectives.
- 441-445: The use of
AddressCodecandValidatorAddressCodecfor address conversions in the context of delegating with power is correctly implemented. This change is consistent with the PR's goal of improving address handling and reduces the potential for errors associated with direct string conversions.x/staking/simulation/operations.go (4)
- 191-194: In
SimulateMsgCreateValidator, the conversion ofaddressfrom bytes to string usingk.ValidatorAddressCodec().BytesToString(address)is correctly implemented. However, ensure that the error handling for this conversion is robust and logs sufficient information for debugging purposes.- 351-354: In
SimulateMsgDelegate, the conversion ofsimAccount.Addressfrom bytes to string usingak.AddressCodec().BytesToString(simAccount.Address)is correctly implemented. It's important to verify that theAddressCodecis properly initialized and configured to handle the address formats used in the Cosmos SDK.- 564-567: In
SimulateMsgCancelUnbondingDelegate, the conversion ofsimAccount.Addressfrom bytes to string usingak.AddressCodec().BytesToString(simAccount.Address)follows the intended refactor. Ensure that theAddressCodecis consistently used across the SDK for address conversions to maintain uniformity and reduce potential errors.- 777-787: In
SimulateMsgRotateConsPubKey, the conversion of consensus addresses from bytes to string usingk.ConsensusAddressCodec().BytesToString(cons)and the subsequent comparison to ensure the new pubkey is different from the current one are correctly implemented. This change enhances the clarity and correctness of address handling. However, it's crucial to ensure that theConsensusAddressCodecis thoroughly tested to handle all edge cases in address conversion.x/staking/keeper/msg_server_test.go (10)
- 73-74: The conversion of
DelegatorAddressandValidatorAddressusings.addressToString(Addr)ands.valAddressToString(ValAddr)respectively is a positive change, aligning with the PR's objective to avoid direct string conversions of addresses. This approach enhances code readability and maintainability by utilizing helper functions for address conversion.- 93-94: Similar to previous comments, using
s.addressToString(Addr)forDelegatorAddressands.addressToString([]byte("invalid"))forValidatorAddressin test cases is consistent with the PR's goal. However, using a byte slice directly with"invalid"forValidatorAddressis a good way to simulate an invalid address scenario in tests.- 113-114: Again, the use of helper functions for address conversion in test cases is noted and aligns with the PR's objectives. This consistency across test cases is beneficial for maintainability.
- 133-134: The consistent use of helper functions for address conversion is observed throughout the test cases, which is a good practice. This approach ensures that the code adheres to the PR's objectives and improves code quality.
- 153-154: Utilizing helper functions for address conversion in test cases is consistently applied here, which is in line with the PR's goals. This consistency is appreciated and enhances the code's maintainability.
- 173-174: The use of helper functions for address conversion is consistently observed, which aligns with the PR's objectives. This practice is beneficial for code readability and maintainability.
- 193-194: The consistent application of helper functions for address conversion in test cases is noted. This approach aligns with the PR's goals and is beneficial for code quality.
- 213-214: Utilizing helper functions for address conversion in test cases is consistently observed, aligning with the PR's objectives. This consistency is beneficial for code quality and maintainability.
- 233-234: The use of helper functions for address conversion in test cases is consistently applied, which aligns with the PR's objectives. This practice enhances code readability and maintainability.
- 257-258: Consistent use of helper functions for address conversion in test cases is observed, aligning with the PR's objectives. This approach is beneficial for code quality.
x/staking/keeper/delegation_test.go (11)
- 47-47: The usage of
s.addressToStringands.valAddressToStringfor address conversion in test cases has been updated. Ensure that these helper functions correctly handle address conversions in line with the PR's objective to improve address handling.- 67-71: The creation of multiple delegations using updated address conversion functions is consistent with the PR's goal. Verify that these changes do not affect the logic of the tests and that the address conversions are correctly implemented.
- 117-117: The use of
s.valAddressToStringfor obtaining the validator's operator address in assertions is noted. Ensure this conversion aligns with the intended address handling improvements and does not introduce any discrepancies in the test outcomes.- 184-184: The delegation test has been updated to use the new address conversion functions. Confirm that the delegation logic remains unaffected by these changes and that the address conversions are accurately performed.
- 382-382: In the
TestUnbondDelegationtest, the delegation creation uses updated address conversion methods. It's important to verify that these changes are in line with the PR's objectives and do not alter the test's expected behavior.- 802-802: The
TestRedelegateToSameValidatortest checks for an error when attempting to redelegate to the same validator. This test uses the updated address conversion functions. Confirm that the logic of detecting redelegation to the same validator is not impacted by these changes.- 824-824: In the
TestRedelegationMaxEntriestest, the delegation setup uses the new address conversion methods. Ensure that the test's logic for checking maximum redelegation entries remains valid and that the address conversions are correctly implemented.- 879-879: The
TestRedelegateSelfDelegationtest uses updated address conversion functions for setting up self-delegation. Verify that these changes align with the PR's goals and do not affect the test's logic for redelegating self-delegation.- 928-928: In the
TestRedelegateFromUnbondingValidatortest, the delegation setup with updated address conversion methods is noted. Confirm that the test's logic for redelegating from an unbonding validator remains accurate and that the address conversions are properly handled.- 997-997: The
TestRedelegateFromUnbondedValidatortest has been updated to use the new address conversion functions. It's crucial to verify that these changes support the PR's objectives without altering the test's intended behavior.- 1196-1196: In the
TestUndelegateWithDustSharetest, the delegation creation using updated address conversion methods is observed. Ensure that these changes are consistent with the PR's goals and do not impact the test's logic for handling dust shares during undelegation.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/staking/keeper/validator_test.go (3 hunks)
Check Runs (11)
tests (03) completed (1)
tests (02) completed (1)
tests (01) completed (1)
test-x-mint completed (1)
test-x-upgrade completed (1)
test-x-staking completed (1)
test-x-slashing completed (1)
test-x-params completed (1)
test-x-group completed (1)
test-x-gov completed (1)
test-x-authz completed (1)
Files skipped from review as they are similar to previous changes (1)
- x/staking/keeper/validator_test.go
| @@ -39,7 +41,7 @@ type WeightedProposalMsg interface { | |||
| MsgSimulatorFn() MsgSimulatorFn // msg simulator function | |||
| } | |||
|
|
|||
| type MsgSimulatorFn func(r *rand.Rand, ctx sdk.Context, accs []Account) sdk.Msg | |||
| type MsgSimulatorFn func(r *rand.Rand, ctx sdk.Context, accs []Account, cdc address.Codec) (sdk.Msg, error) | |||
There was a problem hiding this comment.
Breaking this is quite disruptive for all modules implementing simulation. Is there a way around it? After a quick skim I couldn't find for which simulation it is actually used.
If not, we definitely need a section under Modules > All > Simulation in the UPGRADING.md
There was a problem hiding this comment.
I couldn't find a way to avoid it other than using a global codec for simulation. In the end, either the prefix or the codec must be passed all the way through the stack.
There was a problem hiding this comment.
but for which simulation is that param used? I cannot find it 😅
There was a problem hiding this comment.
right now just for the staking https://github.com/cosmos/cosmos-sdk/pull/19742/files#diff-ccc155cefb86edd2f89c6436d48cde5df2e5734fa9e7df73fd543a29d318b92dR36-R52
But It'll be updated in coming PRs as the rest of MsgSimulatorFn makes use of the String method.
Added a changelog under x/staking. Should add another for simulation? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 54
Configuration used: .coderabbit.yml
Files selected for processing (9)
- UPGRADING.md (1 hunks)
- x/auth/simulation/proposals.go (3 hunks)
- x/bank/simulation/proposals.go (3 hunks)
- x/distribution/simulation/proposals.go (3 hunks)
- x/gov/simulation/proposals.go (2 hunks)
- x/mint/simulation/proposals.go (3 hunks)
- x/protocolpool/simulation/proposals.go (3 hunks)
- x/slashing/simulation/proposals.go (3 hunks)
- x/staking/CHANGELOG.md (1 hunks)
Check Runs (1)
labeler completed (1)
Files skipped from review as they are similar to previous changes (7)
- x/auth/simulation/proposals.go
- x/bank/simulation/proposals.go
- x/distribution/simulation/proposals.go
- x/gov/simulation/proposals.go
- x/mint/simulation/proposals.go
- x/protocolpool/simulation/proposals.go
- x/slashing/simulation/proposals.go
Additional comments: 9
x/staking/CHANGELOG.md (2)
- 44-44: The entry for PR #19742 is clear and concise. However, consider adding a brief explanation of the impact of this change for better clarity.
- 41-47: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [96-96]
The term "key_rotation_fee" is correctly used in the context of staking module changes. However, ensure that this parameter is clearly documented elsewhere to avoid confusion.
UPGRADING.md (7)
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [4-4]
There seems to be a spelling mistake.
Consider changing "SimApp" to "Sim App" if referring to a simulated application, unless "SimApp" is a specific term used in the Cosmos SDK documentation.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [15-15]
This sentence could be improved for clarity.
Consider rephrasing to start with an uppercase letter and ensure proper punctuation for better readability.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [19-19]
There's a missing space after a period, which affects readability.
Please add a space after the period for proper sentence separation.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [30-30]
The term "depinject" might be unclear to some readers.
Verify that "depinject" is a well-understood term within the Cosmos SDK community or consider adding a brief explanation.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [41-41]
There's an extra space before "For depinject users," which might be a typographical error.
Consider removing the extra space for consistency in document formatting.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [45-45]
The term "depinject" appears again without explanation.
Ensure that "depinject" is clearly defined or widely understood in the context of Cosmos SDK documentation.
- 143-158: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [57-57]
The merging of decorators is mentioned without further explanation.
Consider adding a brief explanation or reference to documentation that explains the impact of merging
GasConsumptionDecoratorandIncreaseSequenceDecoratorwithSigVerificationDecorator.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Check Runs (1)
labeler completed (1)
Additional comments: 2
CHANGELOG.md (2)
- 100-100: The description of the change to
MakeTestEncodingConfigis clear and accurate.- 100-100: The explanation for the removal of the basic manager and related functions is clear and provides a good rationale for the change.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: .coderabbit.yml
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- UPGRADING.md (1 hunks)
- simapp/sim_test.go (5 hunks)
- x/staking/CHANGELOG.md (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- simapp/sim_test.go
Additional comments: 2
UPGRADING.md (2)
- 150-156: The
MsgSimulatorFntype has been updated to include anaddress.Codecparameter. This change is crucial for simulations and should be clearly understood by developers.Ensure to update simulation functions across your codebase to accommodate this change.
- 160-160: The introduction of
appmodule.Environmentrepresents a significant architectural change. It's important to understand how to effectively use this new interface for fetching different services within the application.Consider reviewing examples or additional documentation to fully leverage the
appmodule.Environmentinterface.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/staking/keeper/cons_pubkey_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/staking/keeper/cons_pubkey_test.go
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (3)
- CHANGELOG.md (1 hunks)
- UPGRADING.md (1 hunks)
- x/gov/simulation/genesis_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- x/gov/simulation/genesis_test.go
Additional comments: 2
UPGRADING.md (2)
- 150-156: The update to
MsgSimulatorFnis a critical change.Highlight the importance of updating simulation functions to accommodate the new
address.Codecparameter, and provide examples if possible.
- 160-160: The introduction of
appmodule.Environmentis a notable architectural change.Consider providing examples or additional documentation to help developers understand how to use the new
appmodule.Environmentinterface effectively.
Description
ref:
#13140
#7448
This PR removes the use of Address (AccAddress, ValAddress, ConsAddress)
Stringmethod inx/staknigmodule.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