refactor(x/gov)!: remove Accounts.String() #19850
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughWalkthroughThis update focuses on enhancing the address handling within the Cosmos SDK's governance module by removing outdated methods and incorporating address encoding adjustments. It includes changes to function arguments, the introduction of address codecs, and updates to the handling of addresses in proposal submissions and operations. These modifications aim to improve modularity and reduce dependency on global configurations. 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 (
|
|
A refactor may be needed to get rid of this :/ otherwise cosmos-sdk/x/gov/types/v1/msgs.go Lines 113 to 120 in adc3432 |
![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: 10
Configuration used: .coderabbit.yml
Files selected for processing (37)
- tests/integration/gov/abci_test.go (7 hunks)
- tests/sims/gov/operations_test.go (5 hunks)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/client/cli/prompt.go (7 hunks)
- x/gov/client/cli/prompt_test.go (3 hunks)
- x/gov/client/cli/tx.go (3 hunks)
- x/gov/client/cli/tx_test.go (14 hunks)
- x/gov/client/cli/util.go (1 hunks)
- x/gov/client/cli/util_test.go (13 hunks)
- x/gov/client/utils/query.go (1 hunks)
- x/gov/client/utils/query_test.go (6 hunks)
- x/gov/depinject.go (2 hunks)
- x/gov/keeper/common_test.go (7 hunks)
- x/gov/keeper/deposit.go (2 hunks)
- x/gov/keeper/deposit_test.go (9 hunks)
- x/gov/keeper/grpc_query_test.go (41 hunks)
- x/gov/keeper/keeper_test.go (1 hunks)
- x/gov/keeper/msg_server_test.go (56 hunks)
- x/gov/keeper/proposal.go (2 hunks)
- x/gov/keeper/proposal_test.go (10 hunks)
- x/gov/keeper/tally_test.go (17 hunks)
- x/gov/keeper/vote.go (2 hunks)
- x/gov/keeper/vote_test.go (5 hunks)
- x/gov/module.go (1 hunks)
- x/gov/simulation/operations.go (7 hunks)
- x/gov/types/v1/deposit.go (1 hunks)
- x/gov/types/v1/msgs.go (1 hunks)
- x/gov/types/v1/msgs_test.go (4 hunks)
- x/gov/types/v1/params.go (1 hunks)
- x/gov/types/v1/proposal.go (2 hunks)
- x/gov/types/v1/proposals_test.go (3 hunks)
- x/gov/types/v1/vote.go (2 hunks)
- x/gov/types/v1beta1/deposit.go (1 hunks)
- x/gov/types/v1beta1/msgs.go (3 hunks)
- x/gov/types/v1beta1/msgs_test.go (3 hunks)
- x/group/client/cli/prompt.go (4 hunks)
- x/params/client/cli/tx.go (1 hunks)
Additional comments: 151
x/gov/types/v1/deposit.go (1)
- 10-11: The change from
sdk.AccAddresstostringfor thedepositorparameter inNewDepositaligns with the PR's objectives and should simplify address handling. Ensure that address conversion fromsdk.AccAddresstostringis correctly handled throughout the codebase to maintain address formatting and validation integrity.x/gov/types/v1beta1/deposit.go (1)
- 10-11: Identical to the change in
v1/deposit.go, transitioning thedepositorparameter fromsdk.AccAddresstostringinNewDepositis a positive step towards simplifying address handling. Ensure thorough verification of address conversion handling throughout the codebase.x/gov/types/v1beta1/msgs_test.go (1)
- 4-4: The addition of
codectestutilfor address conversion in test cases is appropriate and aligns with the changes in the codebase.x/gov/client/cli/prompt_test.go (1)
- 12-12: The inclusion of
codectestutilfor updated address handling in test cases is a good practice, ensuring that tests align with the codebase changes.x/params/client/cli/tx.go (1)
- 70-73: The modification to retrieve and handle the
fromaddress as a string usingclientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress())is consistent with the PR's goal of standardizing address handling. Ensure that this change does not introduce any issues with address formatting or validation.x/gov/types/v1beta1/msgs.go (1)
- 27-30: The modifications to function signatures, changing parameters from
sdk.AccAddresstostring, are consistent with the PR's objectives to standardize address handling. Ensure that these changes are correctly implemented across the codebase and do not introduce issues with address formatting or validation.Also applies to: 57-58, 82-83, 87-88, 92-93
x/gov/types/v1/msgs_test.go (1)
- 13-13: The addition of
codectestutilfor address conversion in test cases is appropriate and aligns with the changes in the codebase.x/gov/keeper/vote.go (2)
- 71-74: The conversion from
sdk.AccAddressto a string representation usingk.authKeeper.AddressCodec().BytesToString(voterAddr)is a key part of the refactor. Ensure that all instances wherevoterAddrwas previously used are now correctly usingvoterStrAddr.- 87-87: Ensure that the event attributes are correctly using the string representation of the voter address. This is crucial for maintaining consistency in how addresses are represented in events.
x/gov/depinject.go (2)
- 79-82: The conversion of the authority address to a string and the subsequent error handling using a panic is a critical change. While using
panicfor unrecoverable errors is acceptable, ensure that this conversion error is indeed a scenario where the application cannot recover. In most cases, it's preferable to handle errors gracefully.Consider if there's a more graceful way to handle this error that doesn't involve panicking, especially if this operation could fail under normal circumstances.
- 92-92: The usage of the converted authority address (
authorityAddr) in theNewKeeperfunction call aligns with the refactor's goal to handle addresses as strings. This ensures consistency in how addresses are managed within the module.x/gov/types/v1/proposal.go (1)
- 23-29: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [26-49]
The update to the
NewProposalfunction to accept aproposerparameter of typestringand the subsequent assignment to theProposerfield align with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.x/gov/types/v1/vote.go (1)
- 26-27: The update to the
NewVotefunction to accept avoterparameter of typestringis consistent with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.x/gov/types/v1/msgs.go (3)
- 90-91: The update to the
NewMsgDepositfunction to accept adepositorparameter of typestringaligns with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.- 95-96: The update to the
NewMsgVotefunction to accept avoterparameter of typestringis consistent with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.- 100-101: The update to the
NewMsgVoteWeightedfunction to accept avoterparameter of typestringaligns with the refactor's goal to handle addresses as strings. This change ensures consistency in how addresses are managed within the governance module.x/gov/client/utils/query_test.go (5)
- 58-59: The introduction of
cdcOptsfor codec options in theTestGetPaginatedVotesfunction is a good practice, ensuring that codec configurations are explicitly defined and used throughout the test.- 69-70: Converting
sdk.AccAddressinstances to strings before being used in message creation functions is consistent with the refactor's goal to handle addresses as strings. This ensures that the test aligns with the updated module behavior.- 73-74: Similar to the previous comment, converting another
sdk.AccAddressinstance to a string before use is correctly implemented and aligns with the refactor's objectives.- 76-78: The use of string representations for addresses in message creation functions within the test is correctly implemented. This change ensures that the test reflects the updated behavior of the governance module.
- 81-82: Again, the use of string representations for addresses in message creation functions is correctly implemented, ensuring consistency in how addresses are managed within the test.
x/group/client/cli/prompt.go (4)
- 12-12: The addition of the
addressimport from"cosmossdk.io/core/address"is necessary for the changes made in this file, specifically for using theaddress.Codecinterface. This ensures that address encoding and decoding are handled consistently across the module.- 35-37: The modification to the
Promptmethod to accept an additionaladdressCodecparameter and its use in callinggovcli.PromptMetadatais correctly implemented. This change aligns with the refactor's goal to handle addresses as strings and ensures that thePromptmethod can correctly prompt for metadata using the providedaddressCodec.- 75-75: The use of the
addressCodecparameter in callinggovcli.Promptfor setting the proposal message is correctly implemented. This ensures that thePromptmethod can correctly handle message prompting with the providedaddressCodec, aligning with the refactor's objectives.- 146-146: The update to the call to the
Promptmethod inNewCmdDraftProposalto passclientCtx.AddressCodecas theaddressCodecparameter is correctly implemented. This ensures that thePromptmethod receives the correctaddressCodecfor its operations, aligning with the refactor's goal to handle addresses as strings.x/gov/client/utils/query.go (1)
- 123-126: The conversion of
params.Voterfromsdk.AccAddressto a string usingclientCtx.AddressCodec.BytesToString(params.Voter)is a necessary change to align with the PR's objective of handling addresses as strings. This ensures consistency in address handling across the module.x/gov/client/cli/util.go (1)
- 204-208: The update to convert the address using
clientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress())before passing it toReadGovPropCmdFlagsis in line with the PR's goal of standardizing address handling as strings. This change enhances consistency and interoperability within the module.x/gov/keeper/keeper_test.go (1)
- 75-78: The conversion of the governance account address to a string using
suite.acctKeeper.AddressCodec().BytesToString(govAcct)before passing it toNewLegacyMsgServerImplis a good practice. It ensures that the address is handled in a consistent format throughout the tests, aligning with the PR's objectives.x/gov/module.go (1)
- 126-130: Obtaining the module address as a string using
am.accountKeeper.AddressCodec().BytesToString(am.accountKeeper.GetModuleAddress(govtypes.ModuleName))before registering the message server inRegisterServicesis crucial for consistency in address handling. This change aligns with the PR's goal and ensures that addresses are managed uniformly across the module.x/gov/client/cli/tx.go (3)
- 141-146: Converting the proposer's address to a string in
NewMsgSubmitProposalusingclientCtx.AddressCodec.BytesToString(clientCtx.GetFromAddress())before creating a proposal message is aligned with the PR's objectives. This ensures that addresses are handled consistently as strings.- 211-216: Similarly, in
NewCmdSubmitLegacyProposal, converting the proposer's address to a string before creating a legacy proposal message maintains consistency in address handling across the module.- 260-263: In
NewCmdWeightedVote, converting the voter's address to a string before submitting a weighted vote ensures uniform address handling, aligning with the PR's goal of standardizing address management.x/gov/keeper/vote_test.go (1)
- 25-29: Converting addresses to strings using
authKeeper.AddressCodec().BytesToStringin the voting tests enhances readability and maintainability by using string representations for address comparisons. This change aligns with the PR's objectives of standardizing address handling.x/gov/keeper/common_test.go (2)
- 83-94: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [86-100]
The function
mockDefaultExpectationsnow returns an error, which is a good practice for error handling. However, ensure that all callers of this function properly handle the returned error. Failing to do so could lead to ignored errors that might affect the reliability of the tests.Verification successful
The verification process confirms that the error returned by
mockDefaultExpectationsis properly handled by its caller within thex/gov/keeper/common_test.gofile. This aligns with the best practices for error handling, especially in test environments, ensuring the reliability of the tests.* 184-227: > 📝 **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [171-226]Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for calls to mockDefaultExpectations and ensure error handling is implemented. rg --type go 'mockDefaultExpectations\(' --context 2Length of output: 591
The function
trackMockBalanceshas been updated to handle errors more robustly, particularly with address conversions. This is a positive change for error handling. However, ensure that the error messages provided are clear and helpful for debugging. For example, when an error occurs during address conversion, it might be useful to include the problematic address in the error message.- return err + return fmt.Errorf("failed to convert address: %v, error: %w", sender, err)Apply this pattern to all similar error returns within
trackMockBalances.x/gov/client/cli/prompt.go (3)
- 96-106: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [68-103]
The addition of
addressCodecto thePromptfunction and its usage for address conversion is a good practice for handling different address formats. However, ensure that the error handling for address conversion is consistent and provides clear feedback to the user. For example, when an error occurs during address conversion, it might be useful to include more context in the error message.- return data, err + return data, fmt.Errorf("failed to convert authority address: %v, error: %w", types.ModuleName, err)Apply this pattern to all similar error returns within the
Promptfunction.
- 163-170: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [166-193]
The modifications to the
proposalTypemethodPromptto includeaddressCodecand handle address encoding are aligned with the overall changes in this PR. However, ensure that the error messages are informative and guide the user on what went wrong, especially in cases where address conversion might fail.- return nil, metadata, fmt.Errorf("failed to set proposal message: %w", err) + return nil, metadata, fmt.Errorf("failed to set proposal message due to address conversion error: %w", err)This change provides more context in the error message, making it easier for users to understand the issue.
- 217-219: The update to
PromptMetadatato includeaddressCodecis consistent with the changes made in other parts of the file. However, consider adding error handling or validation for the address conversion within this function if applicable. If address conversion is not directly used in this function, ensure that any downstream functions that requireaddressCodecare properly handling potential errors.x/gov/keeper/proposal.go (1)
- 131-136: The conversion of the proposer's address to a string before creating a new proposal is correctly implemented. This change aligns with the PR's objective to handle addresses as strings. No issues found here.
x/gov/keeper/deposit.go (2)
- 168-172: The conversion of the depositor's address to a string before creating a new deposit is correctly implemented. This change is consistent with the PR's objective to handle addresses as strings. No issues found here.
- 254-257: The conversion of the pool module account address to a string before using it in operations is correctly implemented. This ensures consistency in address handling throughout the module. No issues found here.
x/gov/client/cli/tx_test.go (1)
- 71-72: The introduction of
val0StrAddrto store the string representation ofval[0].Addressand its subsequent use in function calls is correctly implemented. This change improves readability and consistency in address handling within the test cases.x/gov/types/v1/params.go (1)
- 263-263: The change to use
addressCodec.StringToBytesfor validating theProposalCancelDestaddress aligns with the PR's objective to handle addresses as strings. This ensures consistency in address handling across the module. No issues found here.x/gov/keeper/proposal_test.go (5)
- 16-16: The addition of
codectestutilimport is appropriate for the new address handling logic introduced in the tests.- 126-136: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [129-142]
The conversion of addresses using
AddressCodec().BytesToStringand the setting of message-based parameters inTestSubmitProposalare significant changes aligning with the PR's objectives to handle addresses as strings.
- 167-167: The test case ensuring an error is thrown when the signer is not the governance account is a good practice for validating proposal submission logic.
- 184-203: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [187-306]
The
TestCancelProposalfunction has been significantly refactored to accommodate string-based address handling and to test various scenarios of proposal cancellation. This includes proper error handling and the validation of expected error messages, which enhances the test coverage for proposal cancellation logic.
- 333-335: The
TestMigrateProposalMessagesfunction correctly utilizes thecodectestutilfor address conversion, ensuring consistency with the new address handling approach. This test validates the migration logic for proposal messages, which is crucial for backward compatibility and data integrity.x/gov/keeper/deposit_test.go (9)
- 43-44: Tracking mock balances with
trackMockBalancesand ensuring no error occurs is a foundational setup step for the deposit tests, ensuring that the test environment is correctly initialized.- 56-59: The conversion of addresses using
AddressCodec().BytesToStringforTestAddrsis consistent with the PR's objectives to handle addresses as strings. This change is crucial for ensuring that the tests align with the updated logic in the governance module.- 88-88: Asserting the depositor's address as a string in the deposit tests is a direct consequence of the transition to string-based address handling. This change ensures that the tests reflect the updated logic in the governance module.
- 101-101: Similar to previous comments, asserting the depositor's address as a string after a second deposit from the same address validates the consistent handling of addresses as strings throughout the deposit logic.
- 113-113: The assertion of the depositor's address as a string after a deposit from a new address further confirms the adherence to string-based address handling in the governance module's deposit logic.
- 136-138: The validation of depositor addresses as strings in the test for deposit iterator functionality is consistent with the overall transition to string-based address handling. This ensures that the deposit logic correctly handles addresses as strings in various contexts.
- 222-223: The setup step for
TestDepositAmountinvolving tracking mock balances and ensuring no error occurs is essential for initializing the test environment correctly. This setup is foundational for testing the deposit amount logic under various scenarios.- 419-426: The handling of the proposal cancellation destination address, including the conversion to a string and the conditional logic based on the test case, demonstrates careful consideration of different scenarios in the deposit logic. This includes handling normal addresses and special addresses like the community pool address.
- 453-457: The conversion of
TestAddrs[0]to a string and its use inChargeDepositaligns with the PR's objectives to handle addresses as strings. This change is crucial for ensuring that the deposit charging logic reflects the updated address handling approach.tests/sims/gov/operations_test.go (4)
- 210-211: The changes correctly handle the conversion of the account address to a string, including proper error handling. This aligns with the PR's objectives of transitioning address handling from
sdk.AccAddresstostring.- 263-263: The replacement of
sdk.AccAddresswith a string for the address parameter inv1.NewProposalaligns with the PR's objectives.- 307-307: The replacement of
sdk.AccAddresswith a string for the address parameter inv1.NewProposalis consistent with the PR's objectives.- 349-349: The replacement of
sdk.AccAddresswith a string for the address parameter inv1.NewProposalaligns with the PR's objectives.x/gov/client/cli/util_test.go (5)
- 24-24: The addition of the
codectestutilimport is necessary for address conversion in tests, aligning with the PR's objectives.- 151-153: The conversion of addresses to strings in
TestParseSubmitProposalis consistent with the PR's objectives, ensuring the tests align with the updated address handling.- 213-214: The update of assertions to use
addrStrinstead of direct address usage inTestParseSubmitProposalis necessary and aligns with the PR's objectives.Also applies to: 218-219, 223-223
- 307-308: The addition of the
fromAddrStrvariable and its usage in tests is correctly adapting the tests to the updated address handling.- 341-341: The conversion of
fromAddrto a string and its usage inMsgSubmitProposalstruct initialization inTestReadGovPropFlagscorrectly reflects the updated address handling.Also applies to: 556-556, 573-573, 623-623, 640-640, 656-656, 673-673
x/gov/simulation/operations.go (7)
- 188-194: The conversion from
sdk.AccAddresstostringusingak.AddressCodec().BytesToString()is correctly implemented. However, it's important to ensure that theGetGovernanceAccountmethod always returns a valid account, as this could potentially returnnil, leading to a panic when callingGetAddress().Ensure that
GetGovernanceAccounthas proper error handling and never returnsnilwithout handling it.
- 253-260: The conversion from
sdk.AccAddresstostringfor the account address inNewMsgSubmitProposalis correctly implemented. This change aligns with the PR's objective to handle addresses as strings. However, ensure that the error fromBytesToStringis handled appropriately everywhere it's used to prevent potential runtime panics.- 355-359: The conversion from
sdk.AccAddresstostringinSimulateMsgDepositis correctly implemented. This is consistent with the PR's goal of transitioning to string-based address handling. It's crucial to ensure that all address conversions are handled consistently across the module to avoid errors.- 429-433: The conversion from
sdk.AccAddresstostringinoperationSimulateMsgVoteis correctly implemented. This change is part of the broader refactor to handle addresses as strings. Consistency in error handling for the address conversion is important to maintain throughout the module.- 496-500: The conversion from
sdk.AccAddresstostringinoperationSimulateMsgVoteWeightedis correctly implemented. This aligns with the refactor's goal to handle addresses as strings. Ensure that the error handling for address conversion is consistent and robust across all instances.- 540-544: The conversion from
sdk.AccAddresstostringinSimulateMsgCancelProposalis correctly implemented. This change supports the PR's objective of transitioning to string-based address handling. It's essential to ensure that error handling for address conversion is consistent across the module.- 555-559: The conversion from
sdk.AccAddresstostringfor the account address inNewMsgCancelProposalis correctly implemented. This is part of the broader effort to handle addresses as strings. Ensure that error handling for the address conversion is consistent and robust across all instances.tests/integration/gov/abci_test.go (7)
- 28-29: The conversion from
sdk.AccAddresstostringusingsuite.AccountKeeper.AddressCodec().BytesToString()is correctly implemented for test setup. This change is necessary for the tests to align with the updated address handling in the governance module. Ensure that all tests are updated accordingly to maintain consistency.- 56-57: The conversion from
sdk.AccAddresstostringfor address handling in tests is correctly implemented. This change ensures that the tests are consistent with the module's updated logic for address handling. It's important to ensure that all tests follow this pattern for address conversion.- 200-202: The conversion from
sdk.AccAddresstostringinTestTickPassedDepositPeriodis correctly implemented. This change aligns with the module's transition to string-based address handling. Consistency in address handling across all tests is crucial for maintaining test reliability.- 301-303: The conversion from
sdk.AccAddresstostringinTestTickPassedVotingPeriodis correctly implemented. This ensures that the tests are in line with the governance module's updated address handling logic. Consistency in address conversion across tests is important for test accuracy.- 383-385: The conversion from
sdk.AccAddresstostringinTestProposalPassedEndblockeris correctly implemented. This change is part of ensuring that the tests reflect the module's transition to string-based address handling. It's important to ensure that all tests are updated to maintain consistency.- 445-447: The conversion from
sdk.AccAddresstostringinTestEndBlockerProposalHandlerFailedis correctly implemented. This ensures that the tests are consistent with the governance module's updated logic for address handling. Consistency in address handling across all tests is crucial for maintaining test reliability.- 545-547: The conversion from
sdk.AccAddresstostringinTestExpeditedProposal_PassAndConversionToRegularis correctly implemented. This change aligns with the module's transition to string-based address handling. It's important to ensure that all tests follow this pattern for address conversion.x/gov/keeper/grpc_query_test.go (14)
- 22-22: The conversion from
sdk.AccAddresstostringusingAddressCodec().BytesToStringis correctly implemented. However, it's not clear wheregovAcctis defined. EnsuregovAcctis properly initialized before this conversion.- 60-60: The use of
govAcctStrinNewLegacyContentindicates a shift towards using string representations for account addresses, aligning with the PR's objectives. This change is correctly applied.- 106-106: Similar to the previous comment, ensure
govAcctis defined and initialized before its conversion togovAcctStr. This pattern is consistent across the file, indicating a systematic approach to handling account addresses as strings.- 197-197: The conversion of
addrs[0]toaddr0Stris correctly implemented. This change is part of the broader refactor to handle addresses as strings. Ensure thataddrsis correctly populated withsdk.AccAddressinstances before this line.- 223-223: The conversion of the governance account address to a string is correctly implemented. This change is consistent with the PR's objectives to handle addresses as strings. Ensure the governance account is correctly retrieved and not nil.
- 287-287: Creating a new deposit with
addr0Stras the depositor's address is correctly implemented, aligning with the refactor to use string representations for addresses. This change is part of the broader effort to standardize address handling.- 414-414: Again, ensure
govAcctis defined and initialized before its conversion togovAcctStr. This repeated pattern across different test suites indicates a comprehensive approach to refactoring address handling.- 461-461: The conversion of
addrs[0]toaddr0Stris correctly implemented, following the PR's objectives. Ensure thataddrsis correctly populated withsdk.AccAddressinstances before this line.- 582-582: Similar to previous comments, ensure
govAcctis defined and initialized before its conversion togovAcctStr. This consistency across test suites supports the PR's objectives.- 705-705: The conversion of
addrs[0]toaddr0Stris correctly implemented, aligning with the refactor to handle addresses as strings. Ensure thataddrsis correctly populated withsdk.AccAddressinstances before this line.- 1120-1120: Ensure
govAcctis defined and initialized before its conversion toaddr0Str. This pattern is consistent across the file, indicating a systematic approach to handling account addresses as strings.- 1226-1226: Similar to previous comments, ensure
govAcctis defined and initialized before its conversion toaddr0Str. This repeated pattern across different test suites indicates a comprehensive approach to refactoring address handling.- 1333-1333: The conversion of
addrs[0]toaddr0Stris correctly implemented, following the PR's objectives. Ensure thataddrsis correctly populated withsdk.AccAddressinstances before this line.- 1436-1436: Ensure
govAcctis defined and initialized before its conversion toaddr0Str. This consistency across test suites supports the PR's objectives.x/gov/keeper/tally_test.go (16)
- 148-154: The conversion from
sdk.AccAddresstostringusingBytesToStringis repeated multiple times across different test cases. Consider refactoring this into a helper function to improve code maintainability and reduce duplication.- 177-183: Similar to the previous comment, the address conversion logic is repeated. Refactoring into a helper function would enhance code readability and maintainability.
- 207-213: Again, the repeated logic for address conversion is observed. Consolidating this logic into a single, reusable function would be beneficial for code cleanliness and maintainability.
- 242-256: The repeated pattern of address conversion is evident here as well. Implementing a helper function for this purpose would streamline the code and improve maintainability.
- 481-484: The use of
require.NoError(t, err)inside a loop for converting validator addresses could potentially halt the test prematurely if an error occurs. While this is generally acceptable in tests to ensure setup correctness, ensure that this behavior is intentional and that the error handling aligns with the test's objectives.- 614-620: The address conversion logic is repeated here. Implementing a helper function to handle this conversion would make the tests cleaner and more maintainable.
- 643-649: As with previous instances, the repeated address conversion logic could benefit from refactoring into a helper function to improve code maintainability.
- 673-679: The pattern of repeating address conversion logic continues. A helper function would enhance code maintainability and readability.
- 708-722: The repeated logic for address conversion is observed again. Refactoring this into a helper function would improve code maintainability.
- 951-954: Similar to a previous comment, ensure that the use of
require.NoError(t, err)inside a loop is intentional and aligns with the test's objectives. This could halt the test prematurely if an error occurs, which is generally acceptable for setup correctness but should be intentional.- 1044-1050: The repeated logic for address conversion is observed. Implementing a helper function for this conversion would improve code maintainability and readability.
- 1248-1254: The address conversion logic is repeated here. A helper function to handle this conversion would make the tests cleaner and more maintainable.
- 1277-1283: As with previous instances, the repeated address conversion logic could benefit from refactoring into a helper function to improve code maintainability.
- 1307-1313: The pattern of repeating address conversion logic continues. A helper function would enhance code maintainability and readability.
- 1342-1356: The repeated logic for address conversion is observed again. Refactoring this into a helper function would improve code maintainability.
- 1501-1504: Ensure that the use of
require.NoError(t, err)inside a loop for converting validator addresses is intentional and aligns with the test's objectives. This could halt the test prematurely if an error occurs, which is generally acceptable for setup correctness but should be intentional.x/gov/keeper/msg_server_test.go (41)
- 29-33: The conversion from
sdk.AccAddresstostringusingsuite.acctKeeper.AddressCodec().BytesToString(addrs[0])and error handling withsuite.Require().NoError(err)is correctly implemented. This change aligns with the PR's objective to handle addresses as strings.- 40-41: Using
govStrAcctandproposerAddrasFromAddressandToAddressinbanktypes.MsgSendis consistent with the refactor to use string representations of addresses. This ensures compatibility with the updated address handling approach.- 70-70: The use of
proposerAddras a string inv1.NewMsgSubmitProposalis correct and aligns with the refactor's goals. This change is part of the broader effort to standardize address handling across the governance module.- 160-160: The error message
"metadata too long"is clear and informative, providing direct feedback about the issue with the proposal's metadata length. This is a good practice for error messages, as it helps developers understand the cause of the error quickly.- 205-205: The test case
"signer isn't gov account"correctly anticipates the scenario where a non-governance account attempts to submit a proposal. This is an important test case to ensure that only authorized accounts can perform governance actions.- 220-220: The test case
"invalid msg handler"checks for the scenario where a proposal message is not recognized by the router. This is crucial for ensuring that only valid messages can be included in proposals, maintaining the integrity of the governance process.- 235-235: The error message
"invalid deposit denom"is clear and directly addresses the issue with the deposited coin's denomination. This specificity in error messages is beneficial for debugging and understanding the constraints of governance actions.- 311-312: Converting the address to a string for the
proposerAddrin theTestSubmitMultipleChoiceProposalfunction is consistent with the refactor's objectives. This ensures that the test aligns with the updated governance module's address handling.- 324-324: The test case for
"empty options"inTestSubmitMultipleChoiceProposalcorrectly identifies a scenario where a multiple-choice proposal is submitted without any options. This test ensures that the governance module enforces the requirement for proposals to have at least two options.- 388-392: The conversion of both
proposerandgovAcctto string addresses (proposerAddrandgovStrAcct) inTestMsgCancelProposalis correctly implemented. This consistency in handling addresses as strings is crucial for the refactor's success.- 396-397: Using string addresses in the
banktypes.MsgSendwithin theTestMsgCancelProposalfunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 404-404: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestMsgCancelProposalfunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 490-493: The conversion of addresses to strings in
TestMsgVotefor bothproposerand the governance account is correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 499-500: Using string addresses in the
banktypes.MsgSendwithin theTestMsgVotefunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 507-507: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestMsgVotefunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 612-618: The test case for
"voter error"inTestMsgVotecorrectly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.- 670-675: The conversion of the governance account to a string address in
TestMsgVoteWeightedis correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 681-682: Using string addresses in the
banktypes.MsgSendwithin theTestMsgVoteWeightedfunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 689-689: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestMsgVoteWeightedfunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 965-966: The conversion of the governance account to a string address in
TestMsgDepositis correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 976-977: Using string addresses in the
banktypes.MsgSendwithin theTestMsgDepositfunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 984-984: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestMsgDepositfunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 1054-1057: The conversion of
depositorto a string address and its use inv1.NewMsgDepositwithin theTestMsgDepositfunction is correctly implemented. This aligns with the refactor's objective to handle addresses as strings, ensuring that the test cases are consistent with the updated governance module.- 1070-1071: The conversion of an address to a string using
codectestutil.CodecOptions{}.GetAddressCodec().BytesToString(...)inTestLegacyMsgSubmitProposalis correctly implemented. This ensures that the legacy tests are also aligned with the refactor's goals of standardizing address handling.- 1113-1113: The use of an empty address string in the test case
"empty proposer"correctly simulates a scenario where a proposal is submitted without a valid proposer address. This test ensures that the governance module enforces the requirement for a valid proposer address in proposals.- 1182-1183: The conversion of the governance account to a string address in
TestLegacyMsgVoteis correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 1193-1194: Using string addresses in the
banktypes.MsgSendwithin theTestLegacyMsgVotefunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 1201-1201: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestLegacyMsgVotefunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 1272-1278: The test case for
"voter error"inTestLegacyMsgVotecorrectly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.- 1329-1334: The conversion of both the governance account and a proposer to string addresses in
TestLegacyVoteWeightedis correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 1340-1341: Using string addresses in the
banktypes.MsgSendwithin theTestLegacyVoteWeightedfunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 1348-1348: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestLegacyVoteWeightedfunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 1534-1539: The test case for
"voter error"inTestLegacyVoteWeightedcorrectly simulates a scenario where an invalid voter address is used. This is important for ensuring that only valid addresses can participate in the voting process, maintaining the integrity of governance actions.- 1594-1599: The conversion of the governance account and a proposer to string addresses in
TestLegacyMsgDepositis correctly implemented. This consistency in address handling is essential for aligning the test cases with the governance module's refactor.- 1605-1606: Using string addresses in the
banktypes.MsgSendwithin theTestLegacyMsgDepositfunction aligns with the refactor's goals. This ensures that the test cases are compatible with the updated address handling in the governance module.- 1613-1613: The use of
proposerAddrinv1.NewMsgSubmitProposalwithin theTestLegacyMsgDepositfunction is correct. This change is part of the broader effort to standardize address handling across the governance module by using string representations.- 1665-1668: The conversion of
depositorto a string address and its use inv1beta1.NewMsgDepositwithin theTestLegacyMsgDepositfunction is correctly implemented. This aligns with the refactor's objective to handle addresses as strings, ensuring that the test cases are consistent with the updated governance module.- 2244-2245: The conversion of an address to a string using
suite.acctKeeper.AddressCodec().BytesToString(suite.addrs[0])inTestMsgSudoExecis correctly implemented. This ensures that the test cases are aligned with the refactor's goals of standardizing address handling.- 2258-2258: The use of
v1.NewMsgVotewithinTestMsgSudoExecand setting it as the sudo-ed message is a valid test case for ensuring that the governance module can perform actions on behalf of other accounts through sudo. This test case is important for verifying the functionality of theMsgSudoExecmessage.- 2218-2224: The setup for the
TestSubmitProposal_InitialDeposittest cases, including the conversion of an address to a string and setting the governance parameters, is correctly implemented. This setup is crucial for testing the governance module's behavior under various conditions related to initial deposit requirements.- 2227-2227: The creation of a
MsgSubmitProposalmessage withinTestSubmitProposal_InitialDepositusing a string address for the proposer is correctly aligned with the refactor's objectives. This ensures that the test cases are consistent with the updated governance module's address handling.
tac0turtle
approved these changes
Mar 25, 2024
sontrinh16
approved these changes
Mar 25, 2024
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (6)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/client/cli/prompt.go (7 hunks)
- x/gov/client/cli/prompt_test.go (3 hunks)
- x/gov/simulation/operations.go (7 hunks)
- x/gov/types/v1/proposal.go (2 hunks)
- x/gov/types/v1beta1/msgs_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (5)
- x/gov/client/cli/prompt.go
- x/gov/client/cli/prompt_test.go
- x/gov/simulation/operations.go
- x/gov/types/v1/proposal.go
- x/gov/types/v1beta1/msgs_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: .coderabbit.yml
Files selected for processing (1)
- x/gov/CHANGELOG.md (1 hunks)
Additional comments: 2
x/gov/CHANGELOG.md (2)
- 66-66: The phrase "take an address.Codec as arguments" in the context of
PromptandPromptMetadatais correctly pluralized, aligning with the subject. However, ensure consistency in documentation by verifying similar constructs throughout the changelog.- 67-67: The correction to "takes a String as an argument" for
SetProposeris accurate and improves the clarity of the documentation by specifying that a single string argument is expected instead of afmt.Stringerinterface.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Comment on lines
+64
to
+67
| * [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method: | ||
| * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`. | ||
| * `Prompt` and `PromptMetadata` take an address.Codec as arguments. | ||
| * `SetProposer` takes a String as an argument instead of a `fmt.Stringer`. |
There was a problem hiding this comment.
The API Breaking Changes section lists several functions with their argument types changed from sdk.AccAddress to string. However, there are duplicate entries for NewMsgVote and NewMsgVoteWeighted. This redundancy should be corrected to maintain clarity and accuracy in the documentation.
- * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal`, `NewMsgVote`, `NewMsgVoteWeighted` now take a string as an argument instead of an `sdk.AccAddress`.
+ * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`.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.
Suggested change
| * [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method: | |
| * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`. | |
| * `Prompt` and `PromptMetadata` take an address.Codec as arguments. | |
| * `SetProposer` takes a String as an argument instead of a `fmt.Stringer`. | |
| * [#19850](https://github.com/cosmos/cosmos-sdk/pull/19850) Removes the use of Accounts String method: | |
| * `NewDeposit`, `NewMsgDeposit`, `NewMsgVote`, `NewMsgVoteWeighted`, `NewVote`, `NewProposal`, `NewMsgSubmitProposal` now take a string as an argument instead of an `sdk.AccAddress`. | |
| * `Prompt` and `PromptMetadata` take an address.Codec as arguments. | |
| * `SetProposer` takes a String as an argument instead of a `fmt.Stringer`. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/gov/CHANGELOG.md (1 hunks)
- x/gov/keeper/deposit.go (2 hunks)
- x/gov/keeper/proposal.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- x/gov/keeper/deposit.go
- x/gov/keeper/proposal.go
Additional comments: 1
x/gov/CHANGELOG.md (1)
- 65-68: The API Breaking Changes section correctly lists the changes related to the removal of
Accounts.String()method and adjustments in argument types for various functions. However, it's essential to ensure that these changes are accurately reflected in the codebase and documentation to avoid confusion.
meetrick
pushed a commit
to meetrick/cosmos-sdk
that referenced
this pull request
Mar 28, 2024
Co-authored-by: son trinh <trinhleson2000@gmail.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This was referenced Apr 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
NewDeposit,NewMsgDeposit,NewMsgVote, among others, to ensure accurate processing.AccountsString method to streamline functionality.NewProposalfunction to accept proposer parameter as a string for consistency.codectestutilinmsgs_test.go.